ENH: Make layout order an initialization parameter of ArrayProxy by effigies · Pull Request #1131 · nipy/nibabel

effigies · 2022-08-20T17:00:27Z

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:

class CArrayProxy(ArrayProxy):
    order = 'C'

Minor in-passing cleanups, such as removing the explicit (object) superclass and moving the test loops to parametrizations.

codecov · 2022-08-20T17:05:38Z

Codecov Report

Merging #1131 (cff42d6) into master (fd5c84b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

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

Impacted Files	Coverage Δ
nibabel/arrayproxy.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

effigies · 2022-08-29T14:47:52Z

Anybody up for a review?

matthew-brett · 2022-08-29T14:51:38Z

Will do, tomorrow ...

matthew-brett

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 be default_order. Is there a good way of going there without breaking people's code?

matthew-brett · 2022-08-30T17:30:18Z

    assert_array_equal(arr[sliceobj] * 2.0 + 1.0, prox[sliceobj])


+@pytest.mark.parametrize("order", ("C", "F"))


Also test order=None?

Does not work for fobj.write(arr.tobytes(order=order)) (currently L198). This is testing override specifically, so I'm not sure "use default" is really relevant.

effigies · 2022-08-30T20:14:12Z

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 be default_order. Is there a good way of going there without breaking people's code?

Could change order to default_order, and then look for an order class variable (hasattr(self.__class__, "order")) in __init__ and raise a DeprecationWarning.

Co-authored-by: Matthew Brett <matthew.brett@gmail.com>

matthew-brett · 2022-08-31T15:03:18Z

Could change order to default_order, and then look for an order class variable (hasattr(self.__class__, "order")) in __init__ and raise a DeprecationWarning.

Nice idea.

effigies · 2022-09-07T12:05:10Z

Matthew, any further comments?

matthew-brett

Oops - sorry - some of these I forgot to add in another review.

matthew-brett · 2022-08-31T15:07:31Z

+                          'to avoid conflict with instance variables.\n'
+                          '* deprecated in version: 5.0\n'
+                          '* will raise error in version: 7.0\n',
+                          DeprecationWarning, stacklevel=2)


I forget the mechanism to time out the deprecation warning ... but shouldn't we put that in here?

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:

matthew-brett · 2022-09-07T12:11:01Z

+    _default_order = 'C'
+
+
+class DeprecatedCArrayProxy(ArrayProxy):


Timed out depraction again (as above)?

This is specifically for testing. The warning/expiration will show up in test_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>

effigies · 2022-09-07T12:43:06Z

+                          'to avoid conflict with instance variables.\n'
+                          '* deprecated in version: 5.0\n'
+                          '* will raise error in version: 7.0\n',
+                          DeprecationWarning, stacklevel=2)


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:

effigies · 2022-09-07T12:44:14Z

+    # Start raising errors when we crank the dev version
+    if Version(__version__) >= Version('7.0.0.dev0'):
+        cm = pytest.raises(ExpiredDeprecationError)
+    else:
+        cm = pytest.deprecated_call()


effigies · 2022-09-07T12:45:03Z

+    _default_order = 'C'
+
+
+class DeprecatedCArrayProxy(ArrayProxy):


This is specifically for testing. The warning/expiration will show up in test_deprecated_order_classvar. When we remove that function, we can remove this class. I'll add a comment...

matthew-brett

LGTM ...

effigies · 2022-09-07T13:47:33Z

Thanks!

effigies force-pushed the enh/arrayproxy_order branch from 6da2600 to 24b8507 Compare August 20, 2022 17:27

matthew-brett reviewed Aug 30, 2022

View reviewed changes

effigies and others added 5 commits August 31, 2022 09:48

ENH: Make layout order an initialization parameter of ArrayProxy

ea68dd1

TEST: Refactor test loops as parameterizations

0155598

TEST: Test order kwarg to ArrayProxy classes

4720932

Update nibabel/arrayproxy.py

df252ca

Co-authored-by: Matthew Brett <matthew.brett@gmail.com>

RF: Change ArrayProxy.order class var to _default_order, deprecate order

3cfe337

effigies force-pushed the enh/arrayproxy_order branch from b7c179c to 8acd26f Compare August 31, 2022 13:54

TEST: Check None and invalid order arguments

dcf9566

effigies force-pushed the enh/arrayproxy_order branch from 8acd26f to dcf9566 Compare August 31, 2022 15:10

matthew-brett reviewed Sep 7, 2022

View reviewed changes

Apply suggestions from code review

cba4607

Co-authored-by: Matthew Brett <matthew.brett@gmail.com>

effigies commented Sep 7, 2022

View reviewed changes

Comment thread nibabel/tests/test_arrayproxy.py

Update nibabel/tests/test_arrayproxy.py

cff42d6

matthew-brett reviewed Sep 7, 2022

View reviewed changes

effigies merged commit 2838c06 into nipy:master Sep 7, 2022

effigies deleted the enh/arrayproxy_order branch September 7, 2022 13:50

effigies added this to the 5.0.0 milestone Jan 3, 2023

		assert_array_equal(arr[sliceobj] * 2.0 + 1.0, prox[sliceobj])


		@pytest.mark.parametrize("order", ("C", "F"))

		_default_order = 'C'


		class DeprecatedCArrayProxy(ArrayProxy):

Conversation

effigies commented Aug 20, 2022

Uh oh!

codecov Bot commented Aug 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

effigies commented Aug 29, 2022

Uh oh!

matthew-brett commented Aug 29, 2022

Uh oh!

matthew-brett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

effigies commented Aug 30, 2022

Uh oh!

matthew-brett commented Aug 31, 2022

Uh oh!

effigies commented Sep 7, 2022

Uh oh!

matthew-brett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matthew-brett left a comment

Choose a reason for hiding this comment

Uh oh!

effigies commented Sep 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Aug 20, 2022 •

edited

Loading