adding a new getitem method #2749

shilpiprd · 2023-03-19T19:01:45Z

I figured that get_item method is most closely related to gradients.py file which is inside dipy/core/gradients.py folder because i saw the bvech bvals method.

Example usage

#since we can call a getItem method by simply using square brackets,
gtab = MyGradientTable(gradients)
gtab_selected = gtab[2:10] # Select b-values from 2 to 10.

Please let me know if I need to make any changes to this method.

arokem · 2023-03-19T19:31:18Z

Yeah - I like it! Could you please add a couple of tests of this new functionality?

codecov · 2023-03-19T20:23:05Z

Codecov Report

Merging #2749 (3fae50e) into master (7ee828b) will increase coverage by 0.00%.
The diff coverage is 82.35%.

❗ Current head 3fae50e differs from pull request most recent head ac107fa. Consider uploading reports for the commit ac107fa to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2749   +/-   ##
=======================================
  Coverage   83.92%   83.92%           
=======================================
  Files         132      132           
  Lines       18464    18493   +29     
  Branches     3014     3050   +36     
=======================================
+ Hits        15495    15521   +26     
- Misses       2226     2228    +2     
- Partials      743      744    +1

Impacted Files	Coverage Δ
dipy/core/sphere.py	`94.04% <ø> (ø)`
dipy/data/fetcher.py	`36.83% <ø> (ø)`
dipy/io/dpy.py	`100.00% <ø> (ø)`
dipy/nn/histo_resdnn.py	`85.55% <ø> (ø)`
dipy/reconst/cross_validation.py	`95.74% <ø> (ø)`
dipy/reconst/dki.py	`96.39% <ø> (ø)`
dipy/reconst/dki_micro.py	`92.30% <ø> (ø)`
dipy/reconst/forecast.py	`90.50% <0.00%> (ø)`
dipy/reconst/fwdti.py	`94.28% <ø> (ø)`
dipy/reconst/ivim.py	`95.17% <ø> (ø)`
... and 16 more

... and 13 files with indirect coverage changes

shilpiprd · 2023-03-19T20:34:07Z

Yeah - I like it! Could you please add a couple of tests of this new functionality?

I've added the tests, please review them.

arokem · 2023-03-20T16:40:10Z

Thanks!

Could you add the tests on this PR? That is, merge or cherry-pick the work you have in #2750 into this branch and then close #2750. It would be best to have the functionality and the tests in the same branch.

shilpiprd · 2023-03-20T16:59:20Z

Sure

skoudoro

Thank you @shilpiprd,

I agree with @arokem concerning the tests. Can you also address the comments below? Thanks!

skoudoro · 2023-03-20T18:06:47Z

dipy/core/gradients.py

+            idx = [idx]
+        elif isinstance(idx, slice):
+            idx = range(*idx.indices(len(self.bvals)))
+        mask = self.bvals[idx] > 200


Why 200? is it possible to get this value in a other way?

You're right. I'll add a 'threshold' parameter to the definition against which values can be compared to create the mask.

skoudoro · 2023-03-20T18:08:48Z

dipy/core/gradients.py

+                                           small_delta=0.01,
+                                           b0_threshold=50,
+                                           atol=1e-2, btens=None)
+    #gave default value for small_delta, big_delta and b0_threshold


Why default value? I recommend to get the value already initialized like self.big_delta , self.small_delta , etc

You're right. I'll change this as well.

shilpiprd · 2023-03-21T09:07:32Z

I updated the getitem method, and realised that the test file needed to be changed as well.I changed the test file in this branch. Should I close the #2750 now?
Please review and let me know if any other changes needs to be made.

arokem · 2023-03-21T20:45:51Z

Yeah. I'll close the other one.

arokem

I've thought about this a bit more. What do you think about indexing with a lower and upper bound on b-values?

dipy/core/gradients.py

arokem · 2023-03-22T05:01:20Z

dipy/core/gradients.py

@@ -210,7 +210,25 @@ def bvecs(self):
        denom = self.bvals + (self.bvals == 0)
        denom = denom.reshape((-1, 1))
        return self.gradients / denom
-
+
+    def __getitem__(self, idx, threshold = 200):


I would set the threshold to be 0 per default.

One more thought is to implement a lower and upper bound, so that if you index:

gtab[:, 100:1000]

you would get a new gradient table with all the b-values between 100 and 1,000. I can see some use-cases for this kind of selection.

I was thinking that we can probably remove the threshold argument and associated code, as it is no longer needed with the new indexing scheme.
Since now we could have: def getitem(self, bmin=0, bmax=np.inf):
And to create mask we can simply use: mask = (self.bvals >= bmin) & (self.bvals <= bmax). Using this to create mask we can remove a lot of unnecessary code as well.
and # Select b-values between 100 and 1000,
gtab = gtab[bmin=100, bmax=1000]
What do you think of this?

I like the idea!

however, a deprecation cycle will be needed. a lot people are using this default behavior so we will have to keep both behavior for 1 year (2-3 releases) at least.

So are you sugggesting something like this?
def getitem(self, bmin=0, bmax=np.inf, threshold=0):
# Use the new behavior if bmin and bmax are provided
if bmin != 0 or bmax != np.inf:
mask = (self.bvals >= bmin) & (self.bvals <= bmax)
else:
# Use the old behavior if bmin and bmax are not provided
mask = self.bvals > threshold

Almost, something like:

from dipy.utils.deprecator import deprecated_params ... @deprecated_params('threshold', None, since='1.17', until='1.19') def getitem(self, bmin=0, bmax=np.inf, threshold=0): if threshold and not bmin: bmin = threshold # priority to bmin in alll the other case mask = (self.bvals >= bmin) & (self.bvals <= bmax) ...

Does it make sense for both of you @arokem? @shilpiprd ? (not tested, need to be checked)

Oh right, so should i create a commit for this? I mean the method looks correct.

yes, update the code and test it to make sure it works as expected.

I modified the gradients file and tests_gradients.py as well. Please review it.

Could somebody please manually trigger the checks on my last commit?

I realised there were slight problems with the previous code. I've corrected them. Could somebody please review them. I think it should pass all the checks now.

Co-authored-by: Ariel Rokem <arokem@gmail.com>

The changes made in the file was not complete This reverts commit b6af6df.

shilpiprd · 2023-03-28T02:49:03Z

The test file is passing all checks. It is failing one check but I can't seem to figure out where the error is because it says PASSED everywhere.
Is there a need to look into this ? or is it mergable?

skoudoro

Hi @shilpiprd,

See below my comment and sorry for the late review.

skoudoro · 2023-03-30T19:31:00Z

dipy/core/gradients.py

-
+
+    @deprecated_params('threshold', None, since='1.17', until='1.19')
+    def getitem(self, bmin=0, bmax=np.inf, threshold=0):


I think, we got lost here.

The goal was to have the slicing capabilities. However, we loose it from your initial implementation.
we can not do anymore gtab[100:200].

we should get back to that. we just need a warning if the slicing is really different from the b0_treshold.

Sorry for that. Can you revert your code, use __get_item__, and in case of a slice, you check if the lower bound is similar to b0_thresold. If yes, nothing to do, if no, just update self.b0_thresold to this value and warn the user

Does it make sense? we do not need(bmin, bmax, thresold)

Sure, you're right. We did get deviated from the original method. I changed it, Did you mean something like this? If you'd agree with this, i'll change the test cases as well.

skoudoro · 2023-03-31T03:37:47Z

dipy/core/gradients.py

+        if bmin != self.b0_threshold:
+            self.b0_threshold = bmin
+            warn("Updating b0_threshold to {} for slicing.".format(bmin), UserWarning, stacklevel=2)


yes, I mean something like this

skoudoro · 2023-03-31T03:38:14Z

dipy/core/gradients.py

@@ -210,7 +211,26 @@ def bvecs(self):
        denom = self.bvals + (self.bvals == 0)
        denom = denom.reshape((-1, 1))
        return self.gradients / denom
-
+
+    def getitem(self, bmin=0, bmax=np.inf):


here it should be __get_item__(self, idx):

Sure, I'll change this

skoudoro · 2023-03-31T03:38:45Z

dipy/core/gradients.py

+            self.b0_threshold = bmin
+            warn("Updating b0_threshold to {} for slicing.".format(bmin), UserWarning, stacklevel=2)
+        # priority to bmin in all other cases
+        mask = (self.bvals >= bmin) & (self.bvals <= bmax)


like you did before, check if slice or just int

I'm sorry I didn't understand. Can u elaborate on what needs to be checked?

Were you referring to bmin value?

Oh, sorry. I understood what you meant.

shilpiprd · 2023-03-31T10:04:18Z

I've added the idx parameter and modified the test file as well. Can someone please review it and allow for workflow checks to run?

shilpiprd · 2023-04-01T02:33:34Z

I noticed that the failing checks are not from the method that I"ve implemented.

skoudoro

Hi @shilpiprd,

Can you address my comments below? Thank you

skoudoro · 2023-03-31T14:39:53Z

dipy/core/gradients.py

@@ -210,7 +211,35 @@ def bvecs(self):
        denom = self.bvals + (self.bvals == 0)
        denom = denom.reshape((-1, 1))
        return self.gradients / denom
-
+
+    def __get_item__(self, idx):


__getitem__ instead

skoudoro · 2023-04-20T17:29:07Z

dipy/core/tests/test_gradients.py

+    gtab = GradientTable(gradients) 
+
+    # Test with a single index
+    gtab_slice1 = gtab.__get_item__(1)


the goal was to do gtab[1] for example

skoudoro · 2023-04-20T17:30:07Z

dipy/core/tests/test_gradients.py

+    assert np.array_equal(gtab_slice1.bvecs, np.array([[1., 0., 0.]]))
+
+    # Test with a range of indices
+    gtab_slice2 = gtab.__get_item__(slice(2, 5))


same, the goal is to do gtab[2:5] and not gtab.__get_item__.. Please can you update

It seems there was a typo in the test file. I changed it, it should pass all the tests now.

shilpiprd · 2023-04-21T08:12:10Z

I don't think it's my method that's failing. It says 1 check failing, but inside it says PASSED for all.

skoudoro · 2023-04-22T03:10:15Z

Thank you @shilpiprd, great job for your first contribution! it works like a charm. merging.

adding a new getitem method

884aa1d

shilpiprd mentioned this pull request Mar 19, 2023

Add a method to slice gtab using bvals (Eg : gtab[bvals>200]) #1073

Closed

shilpiprd mentioned this pull request Mar 19, 2023

Adding tests for gradient.py. #2750

Closed

skoudoro reviewed Mar 20, 2023

View reviewed changes

skoudoro added the type:Enhancement label Mar 20, 2023

Made changes to the getitem method and updated the test file as well.

cc81027

arokem reviewed Mar 22, 2023

View reviewed changes

shilpiprd and others added 6 commits March 22, 2023 13:12

Update dipy/core/gradients.py

2494230

Co-authored-by: Ariel Rokem <arokem@gmail.com>

Removed the Threshold parameter

b6af6df

Revert "Removed the Threshold parameter"

5eb589a

The changes made in the file was not complete This reverts commit b6af6df.

Making changes to getitem

251d221

Adding deprecation and modifying test_file

c30e07f

Changing the test file

673c29e

skoudoro mentioned this pull request Mar 27, 2023

[UPCOMING] Release 1.7.0 - workshop release #2761

Merged

33 tasks

shilpiprd and others added 2 commits March 28, 2023 00:15

modified test file to implement correct test cases

532d994

Removing unnecessary library imports

ecf360c

skoudoro reviewed Mar 30, 2023

View reviewed changes

Reverting to original method

ef55157

skoudoro reviewed Mar 31, 2023

View reviewed changes

Adding the idx parameter and modifying test file.

3fae50e

skoudoro linked an issue Mar 31, 2023 that may be closed by this pull request

Add a method to slice gtab using bvals (Eg : gtab[bvals>200]) #1073

Closed

skoudoro reviewed Apr 20, 2023

View reviewed changes

shilpiprd added 2 commits April 21, 2023 00:09

Made Changes,getitem

da86f75

Removed the typo in test_file

ac107fa

fix pep8 issue

15db1dc

skoudoro merged commit 97eedf5 into dipy:master Apr 22, 2023
24 of 25 checks passed

shilpiprd deleted the new_method branch May 25, 2023 02:23



		@deprecated_params('threshold', None, since='1.17', until='1.19')
		def getitem(self, bmin=0, bmax=np.inf, threshold=0):

adding a new getitem method #2749

adding a new getitem method #2749

Conversation

shilpiprd commented Mar 19, 2023

Example usage

arokem commented Mar 19, 2023

codecov bot commented Mar 19, 2023 • edited

Codecov Report

shilpiprd commented Mar 19, 2023

arokem commented Mar 20, 2023

shilpiprd commented Mar 20, 2023

skoudoro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shilpiprd Mar 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shilpiprd Mar 21, 2023 • edited

Choose a reason for hiding this comment

shilpiprd commented Mar 21, 2023

arokem commented Mar 21, 2023

arokem 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

skoudoro Mar 22, 2023 • edited

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

shilpiprd commented Mar 28, 2023

skoudoro left a comment

Choose a reason for hiding this comment

skoudoro Mar 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skoudoro Mar 31, 2023 • edited

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

shilpiprd commented Mar 31, 2023

shilpiprd commented Apr 1, 2023

skoudoro 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

shilpiprd commented Apr 21, 2023

skoudoro commented Apr 22, 2023

codecov bot commented Mar 19, 2023 •

edited

shilpiprd Mar 21, 2023 •

edited

shilpiprd Mar 21, 2023 •

edited

skoudoro Mar 22, 2023 •

edited

skoudoro Mar 30, 2023 •

edited

skoudoro Mar 31, 2023 •

edited