- Notifications
You must be signed in to change notification settings - Fork269
ENH: Make layout order an initialization parameter of ArrayProxy#1131
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
codecovbot commentedAug 20, 2022 • 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.
Codecov Report
@@ Coverage Diff @@## master #1131 +/- ##======================================= Coverage 91.80% 91.80% ======================================= Files 101 101 Lines 12445 12454 +9 Branches 2430 2433 +3 =======================================+ Hits 11425 11434 +9 Misses 693 693 Partials 327 327
Help us with your feedback. Take ten seconds to tell ushow you rate us. Have a feature suggestion?Share it here. |
6da2600 to24b8507CompareAnybody up for a review? |
Will do, tomorrow ... |
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.
Hmm - I see what you mean, but the interface is now a bit clunky, because there are two ways to override the order. I guess the most direct interface would be to make the class variable bedefault_order. Is there a good way of going there without breaking people's code?
Uh oh!
There was an error while loading.Please reload this page.
| assert_array_equal(arr[sliceobj]*2.0+1.0,prox[sliceobj]) | ||
| @pytest.mark.parametrize("order", ("C","F")) |
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.
Also testorder=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.
Does not work forfobj.write(arr.tobytes(order=order)) (currently L198). This is testing override specifically, so I'm not sure "use default" is really relevant.
Could change |
Co-authored-by: Matthew Brett <matthew.brett@gmail.com>
b7c179c to8acd26fCompare
Nice idea. |
8acd26f todcf9566CompareMatthew, any further comments? |
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.
Oops - sorry - some of these I forgot to add in another review.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| 'to avoid conflict with instance variables.\n' | ||
| '* deprecated in version: 5.0\n' | ||
| '* will raise error in version: 7.0\n', | ||
| DeprecationWarning,stacklevel=2) |
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.
I forget the mechanism to time out the deprecation warning ... but shouldn't we put that in here?
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.
It's a decorator that works for deprecating an entire function/method/class. It does not work for cases like this. This is why I added the check below:
| _default_order='C' | ||
| classDeprecatedCArrayProxy(ArrayProxy): |
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.
Timed out depraction again (as above)?
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 is specifically for testing. The warning/expiration will show up intest_deprecated_order_classvar. When we remove that function, we can remove this class. I'll add a comment...
Co-authored-by: Matthew Brett <matthew.brett@gmail.com>
| 'to avoid conflict with instance variables.\n' | ||
| '* deprecated in version: 5.0\n' | ||
| '* will raise error in version: 7.0\n', | ||
| DeprecationWarning,stacklevel=2) |
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.
It's a decorator that works for deprecating an entire function/method/class. It does not work for cases like this. This is why I added the check below:
| # Start raising errors when we crank the dev version | ||
| ifVersion(__version__)>=Version('7.0.0.dev0'): | ||
| cm=pytest.raises(ExpiredDeprecationError) | ||
| else: | ||
| cm=pytest.deprecated_call() |
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.
... here.
| _default_order='C' | ||
| classDeprecatedCArrayProxy(ArrayProxy): |
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 is specifically for testing. The warning/expiration will show up intest_deprecated_order_classvar. When we remove that function, we can remove this class. I'll add a comment...
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.
LGTM ...
Thanks! |
For#1090, it's been useful to set the proxy order dynamically, rather than subclassing
ArrayProxy. This makes it an initialization argument. I've left it as a class variable on the off-chance that somebody actually uses something like:Minor in-passing cleanups, such as removing the explicit
(object)superclass and moving the test loops to parametrizations.