Don't allow *dop objects to be mutated, and correct the behavior of zero Dop / Sdop#151
Merged
utensil merged 2 commits intopygae:masterfrom Dec 17, 2019
Merged
Conversation
Member
Author
|
Hmm, can't work out how to restart CI here |
eric-wieser
force-pushed
the
fix-dop-mutable-state
branch
2 times, most recently
from
661fcbb to
c843d2b
Compare
eric-wieser
commented
Dec 17, 2019
As discovered in pygaegh-150, printing out a laplacian changes the effect of the operator! In order to ensure immutability, this: * Changes the following from `list` to `tuple`: * `Sdop.terms` * `Pdop.terms` * Changes the following functions to return a new copy, rather than mutating in place _and_ returning themselves: * `Sdop.simplify` * `Pdop.simplify` * `Pdop.factor` * `Dop.consolidate_coefs` * Changes the following undocumented and unused functions to return a new copy, rather than mutating in place and returning `None`: * `Sdop.TSimplify` * `Pdop.TSimplify` * `Sdop.sort_terms`, which has been made private and renamed to `_with_sorted_terms` Crucially, this removes the use of `copy.deepcopy` which was duplicating sympy expressions in weird ways.
`Dop(terms)` is defined as the sum over the terms - the sum over zero terms is zero. To allow `self.terms == []`, we need to not use `zip(*terms)`, which fails when `terms == []`.
eric-wieser
force-pushed
the
fix-dop-mutable-state
branch
from
c843d2b to
79eebfd
Compare
eric-wieser
commented
Dec 17, 2019
Comment on lines
-2027
to
+2039
| if len(args[0]) == 0: # identity Dop | ||
| self.terms = [(S(1),self.Ga.Pdop_identity)] | ||
| else: | ||
| if len(args) == 2: | ||
| if len(args[0]) != len(args[1]): | ||
| raise ValueError('In Dop.__init__ coefficent list and Pdop list must be same length.') | ||
| self.terms = list(zip(args[0],args[1])) | ||
| elif len(args) == 1: | ||
| if isinstance(args[0][0][0], Mv): # Mv expansion [(Mv, Pdop)] | ||
| self.terms = args[0] | ||
| elif isinstance(args[0][0][0], Sdop): # Sdop expansion [(Sdop, Mv)] | ||
| coefs = [] | ||
| pdiffs = [] | ||
| for (sdop, mv) in args[0]: | ||
| for (coef, pdiff) in sdop.terms: | ||
| if pdiff in pdiffs: | ||
| index = pdiffs.index(pdiff) | ||
| coefs[index] += coef * mv | ||
| else: | ||
| pdiffs.append(pdiff) | ||
| coefs.append(coef * mv) | ||
| self.terms = list(zip(coefs, pdiffs)) | ||
| else: | ||
| raise ValueError('In Dop.__init__ args[0] form not allowed. args = ' + str(args)) | ||
| if len(args) == 2: | ||
| if len(args[0]) != len(args[1]): | ||
| raise ValueError('In Dop.__init__ coefficent list and Pdop list must be same length.') | ||
| self.terms = tuple(zip(args[0], args[1])) | ||
| elif len(args) == 1: | ||
| if isinstance(args[0][0][0], Mv): # Mv expansion [(Mv, Pdop)] | ||
| self.terms = tuple(args[0]) | ||
| elif isinstance(args[0][0][0], Sdop): # Sdop expansion [(Sdop, Mv)] | ||
| coefs = [] | ||
| pdiffs = [] | ||
| for (sdop, mv) in args[0]: | ||
| for (coef, pdiff) in sdop.terms: | ||
| if pdiff in pdiffs: | ||
| index = pdiffs.index(pdiff) | ||
| coefs[index] += coef * mv | ||
| else: | ||
| pdiffs.append(pdiff) | ||
| coefs.append(coef * mv) | ||
| self.terms = tuple(zip(coefs, pdiffs)) | ||
| else: | ||
| raise ValueError('In Dop.__init__ length of args must be 1 or 2.') | ||
| raise ValueError('In Dop.__init__ args[0] form not allowed. args = ' + str(args)) | ||
| else: | ||
| raise ValueError('In Dop.__init__ length of args must be 1 or 2.') |
Member
Author
There was a problem hiding this comment.
Note: this is just unindented, hide whitespace for clarity. This biases the coverage report a fair bit - if desirable, I can put this unindent in its own PR to make that effect clearer.
eric-wieser
changed the title
Codecov Report
@@ Coverage Diff @@
## master #151 +/- ##
==========================================
+ Coverage 67.17% 67.23% +0.06%
==========================================
Files 8 8
Lines 4761 4740 -21
==========================================
- Hits 3198 3187 -11
+ Misses 1563 1553 -10
Continue to review full report at Codecov.
|
eric-wieser
added
the
component: differential operators
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As discovered in gh-150, printing out a laplacian changes the effect of the operator!
In order to ensure immutability, this:
listtotuple:Sdop.termsPdop.termsSdop.simplifyPdop.simplifyPdop.factorDop.consolidate_coefsNone:Sdop.TSimplifyPdop.TSimplifySdop.sort_terms, which has been made private and renamed to_with_sorted_termsCrucially, this removes the use of
copy.deepcopywhich was duplicating sympy expressions in weird ways.This needs a second commit to make tests pass, as it exposes another bug:
Correct the meaning of
Dop([])andSdop([]), which is zero, not oneDop(terms)is defined as the sum over the terms - the sum over zero terms is zero.To allow
self.terms == [], we need to not usezip(*terms), which fails whenterms == [].xref gh-141, which was also a mutability issue