Remove commented-out function definitions and delete files with no executable code #452

rileyjmurray · 2024-06-07T12:16:29Z

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:

We can/should keep simple reference implementations of functions that are important but very complicated.
- Example: there's an implementation of Levenberg-Marquardt from Wikipedia that I've kept. I think this is valuable because our actual LM code is very complicated, so having a reference is helpful for maintainability.
We could keep text that's related to memory profiling.
- Rationale: The question of whether or not to bother trying to profile memory usage in a certain location requires a fair amount of expertise, so I think keeping this kind of text could serve as a flag for suspected hot code paths.
Commented-out pyx code should be removed unless we have a very good reason for keeping it.

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

Removed some unused imports.

…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.

I accidentally put down the wrong directory for temp testing files in the RB testing code.

rileyjmurray · 2024-09-30T15:19:24Z

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:

TODOs can be moved to GitHub issues. You can paste code in the GitHub issue as a starting point, for later reference.

You can replace massive comment-out code blocks with a short comment, like the following

# NOTE: The FancyClass is admittedly very complicated. We used to have an equivalent SimpleClass, but
# for maintainability reasons we removed it. The old SimpleClass can be found at
# https://github.com/sandialabs/pyGSTi/blob/d27e4e688a64914a0b7aaec1de91aa56f7ce70c2/pygsti/drivers/longsequence.py#L153

-- I've used a dummy link for reference.

If you really want to keep a commented-out code block, please at least add your name and date so it's obvious who made the call to keep it and when. (There's semi-automated tools for this, but they aren't as reliable as old-fashioned comments.)
Use triple-ticks or triple-quotes for gigantic comments, rather than hundreds of lines that start with the single-line comment character #. Using the former styles makes it possible to collapse irrelevant text in most code editors.

pcwysoc

I have no objection to the removed comments on the files for which I'm a code owner. Someone may wish to put the code for the old RB plot somewhere however if it still has utility. I defer to @jordanh6 or @tjproct on this.

adhumu

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!

pygsti/algorithms/germselection.py

coreyostrove · 2024-10-23T20:30:35Z

pygsti/baseobjs/errorgenbasis.py

@@ -79,11 +79,6 @@ def label_index(self, label, ok_if_missing=False):
            return None
        return self._label_indices[label]

-    #@property


Note to self: Check whether or not I resuscitated this code or not on the error generator propagation branch. If so this shouldn’t be deleted here (will probably create stuff to clean up upon merging).

pygsti/baseobjs/errorgenbasis.py

coreyostrove · 2024-10-23T20:53:48Z

pygsti/baseobjs/polynomial.py

@@ -558,574 +543,6 @@ def to_rep(self):  # , max_num_vars=None not needed anymore -- given at __init__
 Polynomial = FASTPolynomial


-# class SLOWPolynomial(dict):  # REMOVE THIS CLASS (just for reference)


Deleting this looks fine, but with this deleted we should rename the other class in this file as simply Polynomial, instead of FASTPolynomial (which is how it is currently being aliased anyhow).

TODO: turn the slow polynomial into a unit test for FASTPolynomial (to be Polynomial)

coreyostrove · 2024-10-23T20:57:43Z

pygsti/evotypes/densitymx/opreps.pyx

@@ -727,24 +727,6 @@ cdef class OpRepExpErrorgen(OpRep):
        return _copy.deepcopy(self)  # I think this should work using reduce/setstate framework TODO - test and maybe put in base class?


-#TODO: can add this after creating OpCRep_IdentityPlusErrorgen if it seems useful
-#cdef class OpRepIdentityPlusErrorgen(OpRep):


Which rep type currently covers the identity plus error generator object? Is this already covered, or are we deficient in this regard in which case should this be re-enabled?

+1 to look into this

pygsti/modelmembers/povms/denseeffect.py

coreyostrove · 2024-10-23T22:17:08Z

pygsti/models/model.py

Might need to merge in latest develop. This code was merged in with #482 so shouldn’t be showing up in the diff.

Weird. I just tried to rebase from develop, and git says that this branch is already up to date. I'll poke around.

I think the GitHub summary of changes might be messed up. I experimented by creating a copy of the current develop branch and then merging in this branch to that copy. The summary of changes in the command line interface didn't include additions to this file.

pygsti/tools/fogitools.py

coreyostrove · 2024-10-23T22:32:26Z

test/unit/objects/test_prefixtable.py

I will likely be adding real tests for this module as part of a pending PR I have. If this is a full deletion let’s keep this file for now to avoid merge issues.

coreyostrove · 2024-10-23T22:39:05Z

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.

rileyjmurray · 2024-10-24T13:22:29Z

pygsti/extras/interpygate/process_tomography.py

Not sure why I made this change. Reverting.

sserita

2/3 of the way check-in

sserita · 2024-11-12T17:10:59Z

pygsti/algorithms/mirroring.py

@@ -21,92 +21,6 @@

 from . import randomcircuit as _rc

-# ### TODO: THIS IS TIMS OLD CODE WHICH SHOULD PERHAPS ALSO BE AN OPTION IN THE `CREATE_MIRROR_CIRCUIT` FUNCTION


Tim's old mirroring code.

sserita · 2024-11-12T17:12:49Z

pygsti/algorithms/randomcircuit.py

@@ -198,139 +198,6 @@ def find_all_sets_of_compatible_two_q_gates(edgelist, n, gatename='Gcnot', aslab
    return co2Qgates


-# TJP: I am not aware this is ever used anymore, and it's functionality is possibly even included in the


More Tim RC code

sserita · 2024-11-12T17:13:20Z

pygsti/algorithms/rbfit.py

@@ -17,132 +17,6 @@
 from pygsti.tools import rbtools as _rbt


-# Obsolute function to be deleted.


Tim RB fitting.

sserita · 2024-11-12T17:16:30Z

pygsti/baseobjs/polynomial.py

@@ -558,574 +543,6 @@ def to_rep(self):  # , max_num_vars=None not needed anymore -- given at __init__
 Polynomial = FASTPolynomial


-# class SLOWPolynomial(dict):  # REMOVE THIS CLASS (just for reference)


TODO: turn the slow polynomial into a unit test for FASTPolynomial (to be Polynomial)

sserita · 2024-11-12T17:19:05Z

pygsti/evotypes/chp/statereps.py

-#            + self.op_rep.chp_ops(seed_or_state=seed_or_state)
-
-# TODO: Untested, only support computational and composed for now
-#class StateRepTensorProduct(StateRep):


For Stefan to check out

sserita · 2024-11-12T17:42:42Z

pygsti/modelmembers/povms/denseeffect.py

-
-
-#UNUSED - I think we can remove this
-#class DensePOVMEffect(_POVMEffect):


Note: DensePOVMEffect reference

sserita · 2024-11-12T17:43:24Z

pygsti/modelmembers/povms/marginalizedpovm.py

@@ -207,85 +207,6 @@ def __reduce__(self):
                                   self.sslbls_after_marginalizing),
                {'_gpindices': self._gpindices})  # preserve gpindices (but not parent)

-    #May need to implement this in future if we allow non-static MarginalizedPOVMs


Note: non-static MarginalizedPOVM ideas

sserita · 2024-11-12T17:45:01Z

pygsti/modelmembers/term.py

@@ -272,16 +272,6 @@ def __mul__(self, x):
    def __rmul__(self, x):
        return self.__mul__(x)

-    #Not needed - but we would use this if we changed


sserita · 2024-11-12T17:47:44Z

pygsti/modelpacks/stdtarget.py

@@ -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?


This was used for modelpack generation. Fine to remove but note TermForwardSim caching for model packs

sserita · 2024-11-12T17:49:03Z

pygsti/models/memberdict.py

-    #    items = [(k,v) for k,v in self.iteritems()]
-    #    return (_PrefixOrderedDict, (self._prefix, items), None)
-
-    """


Remove reduce, but leave docstring

tjproct · 2024-11-12T18:05:55Z

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: ***@***.***>

rileyjmurray · 2024-11-12T22:19:15Z

Nah, you're good, Tim. No action items for you!

sserita

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!

sserita · 2024-11-21T00:03:17Z

pygsti/optimize/optimize.py

@@ -662,90 +662,6 @@ def _evaluate(individual):
    return solution


-#def fmin_homebrew(f, x0, maxiter):


Mark that we are removing some of Erik's experimental optimizer algorithms

sserita · 2024-11-21T00:04:19Z

pygsti/protocols/vb.py

-#class PassStabilityTest(_proto.Protocol):
-#    pass
-
-# Commented out as we are not using this currently. todo: revive or delete this in the future.


Mark that we are removing VBGrid code that may be revived later

sserita · 2024-11-21T00:05:33Z

pygsti/report/workspaceplots.py

-#This older version on an RB decay plot contained a lot more theory detail
-# compared with the current one - so we'll keep it around (commented out)
-# in case we want to steal/revive pieces of it in the future.
-#class OLDRandomizedBenchmarkingPlot(WorkspacePlot):


Mark RB plotting that may want to be revived later

sserita · 2024-11-21T00:07:27Z

pygsti/tools/rbtheory.py

@@ -458,255 +458,6 @@ def R_matrix(model, group, group_to_model=None, weights=None):  # noqa N802

    return R

-### COMMENTED OUT SO THAT THIS FILE DOESN'T NEED "from .. import construction as _cnst".
-### THIS SHOULD BE ADDED BACK IN AT SOME POINT.
-# def exact_rb_asps(model, group, m_max, m_min=0, m_step=1, success_outcomelabel=('0',),


More RB tools in case we need to revive them.

sserita · 2024-11-21T00:12:42Z

test/unit/objects/test_prefixtable.py

@@ -1,62 +0,0 @@
-import numpy as np


As Corey mentioned, keep this file for now to avoid merge issues with his branch.

rileyjmurray mentioned this pull request Jun 7, 2024

legacyio.py contains no executable code. #424

Closed

rileyjmurray marked this pull request as ready for review September 24, 2024 18:11

rileyjmurray requested review from a team, pcwysoc, kmrudin, kevincyoung and adhumu as code owners September 24, 2024 18:11

rileyjmurray requested a review from sserita September 24, 2024 18:11

rileyjmurray and others added 20 commits September 30, 2024 10:37

initial attempt at removing out unneeded commented-out function defin…

10ba164

…itions. Likely over-zealous.

Allow SIGINT set to be skipped via env variable.

0a1b0ca

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.

Logic bugfix

dd7bfe2

Add caching for spam-free circuit expansion

6383cc3

Cache the expanded SPAM-free circuits to reduce recomputing things unnecessarily.

Minor COPA Layout __init__ tweaks

9d57884

Some minor performance oriented tweaks to the init for COPA layouts.

Refactor some OrderedDicts into regular ones

aa9519d

Refactor some of the ordered dictionaries in matrix layout creation into regular ones.

Start the process of adding caching to MDC store creation

957facd

Start adding infrastructure for caching things used in MDC store creation and for plumbing in stuff from layout creation.

Fix dataset bug

8d5cfc7

Fix a bug I introduced in dataset indexing into something that could be None.

Another minor bugfix caught by testing

cd653b3

Another minor bug caught by testing.

Another minor bugfix caught by testing

fd1e8d1

Faster implementation of __getitem__

1ee6112

Improve the performance of __getitem__ when indexing into static circuits by making use of the _copy_init code path.

Implement caching for map layout creation

1c83f9b

Implement caching of circuit structures tailored to the map forward simulator's requirements.

Fix bugs in new extract_labels implementation

7a38689

Finish refactoring expand_instruments_and_separate_povm

070c2f4

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

8f80e31

Refactor expand_instruments_and_separate_povm to use the multi-circuit version under the hood to reduce code duplication.

Corey Ostrove and others added 14 commits September 30, 2024 10:44

Unrelated RB testing fix

c6ed8ee

I accidentally put down the wrong directory for temp testing files in the RB testing code.

Make test_rb paths absolute.

9ec8c87

Fix beta tests.

599faa0

Merge resolution with #488.

4d40eb7

main changes (breaks some calling functions elsewhere)

bb9a278

check in

cf7dcd1

remove change that wasnt strictly in-scope for the PR

e7b99e9

remove changes that werent strictly necessary

b0b6d7d

tests pass

ec12bdb

remove is_normal function

a156511

add a comment and remove unused imports

362788f

interpygate helpers

a67f17f

remove safe_dot

e79b2af

rebase step 1

2cdfe87

rileyjmurray force-pushed the remove-unused-function-defs branch from 01f3ccc to 2cdfe87 Compare September 30, 2024 14:49

found lots of other commented-out function definitions

024d553

pcwysoc approved these changes Sep 30, 2024

View reviewed changes

adhumu approved these changes Oct 2, 2024

View reviewed changes

coreyostrove requested changes Oct 23, 2024

View reviewed changes

add back commented-out function per request from Corey

5aa5cb7

rileyjmurray commented Oct 24, 2024

View reviewed changes

pygsti/extras/interpygate/process_tomography.py Outdated

Copy link

Contributor Author

rileyjmurray Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why I made this change. Reverting.

rileyjmurray added 2 commits October 24, 2024 09:25

revert out-of-scope and unnecessary change

344670d

restore fogitools to state on develop

00982b2

sserita reviewed Nov 12, 2024

View reviewed changes

rileyjmurray mentioned this pull request Nov 19, 2024

test coverage for slow evotype classes #505

Open

sserita requested changes Nov 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove commented-out function definitions and delete files with no executable code #452

Remove commented-out function definitions and delete files with no executable code #452

rileyjmurray commented Jun 7, 2024 •

edited

Loading

rileyjmurray commented Sep 30, 2024

pcwysoc left a comment

adhumu left a comment

coreyostrove Oct 23, 2024

coreyostrove Oct 23, 2024

sserita Nov 12, 2024

coreyostrove Oct 23, 2024

sserita Nov 12, 2024

coreyostrove Oct 23, 2024

rileyjmurray Oct 24, 2024

rileyjmurray Oct 24, 2024

coreyostrove Oct 23, 2024

coreyostrove commented Oct 23, 2024

rileyjmurray Oct 24, 2024

sserita left a comment

sserita Nov 12, 2024

sserita Nov 12, 2024

sserita Nov 12, 2024

sserita Nov 12, 2024

sserita Nov 12, 2024

sserita Nov 12, 2024

sserita Nov 12, 2024

sserita Nov 12, 2024

sserita Nov 12, 2024

sserita Nov 12, 2024

tjproct commented Nov 12, 2024 via email •

edited by rileyjmurray

Loading

rileyjmurray commented Nov 12, 2024 via email •

edited

Loading

sserita left a comment

sserita Nov 21, 2024

sserita Nov 21, 2024

sserita Nov 21, 2024

sserita Nov 21, 2024

sserita Nov 21, 2024

		@@ -558,574 +543,6 @@ def to_rep(self): # , max_num_vars=None not needed anymore -- given at __init__
		Polynomial = FASTPolynomial


		# class SLOWPolynomial(dict): # REMOVE THIS CLASS (just for reference)

		@@ -21,92 +21,6 @@

		from . import randomcircuit as _rc

		# ### TODO: THIS IS TIMS OLD CODE WHICH SHOULD PERHAPS ALSO BE AN OPTION IN THE `CREATE_MIRROR_CIRCUIT` FUNCTION

		@@ -198,139 +198,6 @@ def find_all_sets_of_compatible_two_q_gates(edgelist, n, gatename='Gcnot', aslab
		return co2Qgates


		# TJP: I am not aware this is ever used anymore, and it's functionality is possibly even included in the

		@@ -17,132 +17,6 @@
		from pygsti.tools import rbtools as _rbt


		# Obsolute function to be deleted.



		#UNUSED - I think we can remove this
		#class DensePOVMEffect(_POVMEffect):

		@@ -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?

		@@ -662,90 +662,6 @@ def _evaluate(individual):
		return solution


		#def fmin_homebrew(f, x0, maxiter):

Remove commented-out function definitions and delete files with no executable code #452

Are you sure you want to change the base?

Remove commented-out function definitions and delete files with no executable code #452

Conversation

rileyjmurray commented Jun 7, 2024 • edited Loading

rileyjmurray commented Sep 30, 2024

pcwysoc left a comment

Choose a reason for hiding this comment

adhumu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreyostrove commented Oct 23, 2024

Choose a reason for hiding this comment

sserita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjproct commented Nov 12, 2024 via email • edited by rileyjmurray Loading

rileyjmurray commented Nov 12, 2024 via email • edited Loading

sserita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rileyjmurray commented Jun 7, 2024 •

edited

Loading

tjproct commented Nov 12, 2024 via email •

edited by rileyjmurray

Loading

rileyjmurray commented Nov 12, 2024 via email •

edited

Loading