-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove commented-out function definitions and delete files with no executable code #452
base: develop
Are you sure you want to change the base?
Conversation
…itions. Likely over-zealous.
These changes address unexpected behavior that can occur when manually adding an operation without then manually rebuilding the parameter vector. When this happens it is possible for the Model's internal attributes to fall out of sync with those of it's child objects. Now we check for the need to rebuild the parameter vector every time.
This is relevant when attempting to use Dask outside of pyGSTi, where signals cannot be set in the workers. Setting the PYGSTI_NO_CUSTOMLM_SIGINT env variable now skips this behavior.
A bug in the parameter label handling code was causing parameter labels to explode exponentially in size when _rebuild_paramvec was caused, leading to major memory issues. This now makes it so that the value of _paramlbls is fixed to that of the underlying operations and adds a new version of the parameter_labels property that goes through the interposer (making the interposer labels something generated on demand). Also add a threshold for coefficients printing in the LinearInterposer to avoid obnoxious labels.
The creation of COPA layouts relies on a number of specialized circuit structures which require non-trivial time to construct. In the context of iterative GST estimation with nested circuit lists (i.e. the default) this results in unnecessarily repeat construction of these objects. This is an initial implementation of a caching scheme allowing for more efficient re-use of these circuit structures across iterations.
Cache the expanded SPAM-free circuits to reduce recomputing things unnecessarily.
Adds a new method to OpModel that allows for doing instrument expansion and povm expansion in bulk, speeding things up be avoiding recomputation of shared quantities. Also adds a pipeline for re-using completed or split circuits (as produced by the related OpModel methods) for more efficient re-use of done work.
Some minor performance oriented tweaks to the init for COPA layouts.
Refactor some of the ordered dictionaries in matrix layout creation into regular ones.
Start adding infrastructure for caching things used in MDC store creation and for plumbing in stuff from layout creation.
Performance optimization for the method for adding omitted frequencies to incorporate caching of the number of outcomes per circuit (which is somewhat expensive since it goes through the instrument/povm expansion code). Additionally refactor some other parts of this code for improved efficiency. Also makes a few minor tweaks to the method for adding counts to speed that up as well. Can probably make this a bit faster still by merging the two calls to reduce redundancy, but that is a future us problem. Additionally make a few microoptimizations to the dataset code for grabbing counts, and to slicetools adding a function for directly giving a numpy array for a slice (instead of needing to cast from a list). Miscellaneous cleanup of old commented out code that doesn't appear needed any longer.
Fix a bug I introduced in dataset indexing into something that could be None.
Another minor bug caught by testing.
Improve the performance of __getitem__ when indexing into static circuits by making use of the _copy_init code path.
Implement caching of circuit structures tailored to the map forward simulator's requirements.
This finishes the process of refactoring expand_instruments_and_separate_povm from a circuit method to a method of OpModel.
Refactor expand_instruments_and_separate_povm to use the multi-circuit version under the hood to reduce code duplication.
01f3ccc
to
2cdfe87
Compare
Hi @pcwysoc, @kmrudin, @kevincyoung, @adhumu, and @sserita. This pull request is a big spring-cleaning for commented-out code. You're all designated as code-owners for files that are affected by this pull request. Please review the changes for files you "own" and let me know if you object to removing specific blocks of commented-out code. Things to keep in mind:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto what Piper said. Stefan and I were working on changing parts of ForwardSims in a different branch and on my local computer. This part of the code is still in development. I'm happy to approve these changes, thanks Riley!
Whoops, forgot to add the top-level comment for my review. TLDR: Good work @rileyjmurray. It will be a while till most of us can vie for the crown of most lines written, but at this pace you’re definitely in the running for most lines of pyGSTi deleted! I’ll defer judgement to other code owners for stuff they manage (i.e. a lack of comment may just mean I am waiting for one of them to decide whether to chime in rather than explicit approval), but have added a few comments regarding code I would like to keep, as well as a few minor suggestion for additional deletion related clean-up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2/3 of the way check-in
# + self.op_rep.chp_ops(seed_or_state=seed_or_state) | ||
|
||
# TODO: Untested, only support computational and composed for now | ||
#class StateRepTensorProduct(StateRep): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Stefan to check out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sserita did you look into this? If not, could you open a GitHub issue articulating the question that needs investigation?
@@ -42,127 +42,6 @@ def _get_cachefile_names(std_module, param_type, simulator, py_version): | |||
raise ValueError("No cache files used for param-type=%s" % param_type) | |||
|
|||
|
|||
# XXX is this used? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was used for modelpack generation. Fine to remove but note TermForwardSim caching for model packs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, @sserita, would you like the source code to include a comment of some kind given that we'll be removing this?
# items = [(k,v) for k,v in self.iteritems()] | ||
# return (_PrefixOrderedDict, (self._prefix, items), None) | ||
|
||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove reduce, but leave docstring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sserita are you sure? The docstring doesn't seem to apply to this class. It applies somewhat to the class below, but that class has more detailed docs.
I’m guessing I’m meant to be doing something? My main reaction to this pull request is/was “argh I’m pretty sure some of that commented out code should be revived, but I have no time to even think about this”. That’s probably very unhelpful, but that’s how it is right now. -T
From: Stefan Seritan ***@***.***>
Date: Tuesday, November 12, 2024 at 11:53 AM
To: sandialabs/pyGSTi ***@***.***>
Cc: Proctor, Timothy James ***@***.***>, Mention ***@***.***>
Subject: [EXTERNAL] Re: [sandialabs/pyGSTi] Remove commented-out function definitions and delete files with no executable code (PR #452)
<<< Insert lots of content here >>>
—
Reply to this email directly, view it on GitHub<#452 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AFQEBIBJDOEXVBODCBD527L2AI5VXAVCNFSM6AAAAABI6RXALOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMZQGIZDSOBYGI>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Nah, you're good, Tim. No action items for you!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The majority of my comments are just personal notes for things we should highlight/bookmark in the PR in case we want to come back and revive them later.
Some of my comments are things I would like to be left in, and at least one is an item I need to dig further into to decide.
Thanks for your hard work in help clean up the code, it is super appreciated!
…e old commented-out SLOWPolynomial class and move it into test/objects/test_polynomial.py
I've made some changes based on comments above. Some questions remain. The ball is now simultaneously in Corey's, Stefan's, Tim's, and Jordan's courts. |
This PR aims to resolve #428.
At time of opening this PR, the changes might be over-zealous, so I'm happy to roll some changes back. That said, I suggest we be aggressive in removing this kind of unused text. Here's a proposal for how to decide what to actually keep and what to remove:
Another source of criteria for figuring out what to keep is to look for a short note at the beginning of some commented-out code block. Often there are notes that say "FUTURE," "Removed," "unused," "REMOVE," "Debugging," etc...
Other, miscellaneous changes