Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Make all transforms copiable (and thus scales, too).#19281
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
timhoffm left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
AFAICS the tests do not test the behavioral difference between copy and deepcopy. The specific behavior of both should be exercised in the tests.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
e62d61e to007afceCompareThere 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 looks like it would be good if it is robust. I'm still not clear what is going on well enough to review it though - what happens when people start using these copies in the wild? Also, what is the difference withtransform.frozen()?
| # propagate back to `c`, i.e. we need to clear the parents of `a1`. | ||
| other._parents = {} | ||
| # If `c = a + b; c1 = deepcopy(c)`, this creates a separate tree | ||
| # (`c1 = a1 + b1`) so nothing needs to be done. |
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.
OK, so with a deepcopy the tranform is frozen, or at least to the point where someone would need to dig out the children? Does thecopy.deepcopy above really copy the children over?
I agree tests are needed. I'll work on these in the coming days... |
f9f8d41 to3a63e5eCompareNow with tests... |
Uh oh!
There was an error while loading.Please reload this page.
3a63e5e to7b48c86CompareThere 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.
Thisseems fine to me. Has the desired effect of making norms copy-able. I don't think it will have any negative consequences for end-users, though I don't now all the ways our users might use transforms....
Uh oh!
There was an error while loading.Please reload this page.
7b48c86 tob12eb12Compare| def test_scale_deepcopy(): | ||
| sc = mscale.LogScale(axis='x', base=10) | ||
| sc2 = copy.deepcopy(sc) | ||
| assert str(sc.get_transform()) == str(sc2.get_transform()) |
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.
As a rough check that there is no shared state, I'd add
assert sc._transform is not sc2._transformor, if you're uncomfortable with testing private attributes:
assert sc.get_transform() is not sc2.get_transform()But personally, I'd test the internal state here, becauseget_transform()could be written to return copies, so that testingget_transform() include an additional assumption.
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.
yes (although this revealed the need to separately implementdeepcopy for transforms that just return self in frozen()).
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 test found an implementation flaw. It's working 😄
Uh oh!
There was an error while loading.Please reload this page.
b12eb12 to921e945Compare
PR Summary
Closes#18119; more general than#18126?
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).