Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RF - Direction getters naming #2033

Merged
merged 10 commits into from Apr 15, 2020
Merged

Conversation

gabknight
Copy link
Contributor

@gabknight gabknight commented Jan 11, 2020

Change direction getter naming in preparation of new direction getters based on peaks object (PR in prep).

  • class BaseDirectionGetter -> BasePmfDirectionGetter
  • file peak_direction_getter.pyx -> eudx_direction_getter.pyx
  • file test_peak_dg.py -> test_eudx_dg.py

@codecov
Copy link

codecov bot commented Jan 11, 2020

Codecov Report

Merging #2033 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2033      +/-   ##
==========================================
+ Coverage   91.21%   91.22%   +0.01%     
==========================================
  Files         250      252       +2     
  Lines       31803    31832      +29     
  Branches     3340     3340              
==========================================
+ Hits        29009    29039      +30     
  Misses       2074     2074              
+ Partials      720      719       -1     
Impacted Files Coverage Δ
dipy/segment/tests/test_bundles.py 89.52% <0.00%> (-0.96%) ⬇️
...ection/tests/test_closest_peak_direction_getter.py 100.00% <0.00%> (ø)
dipy/reconst/peak_direction_getter.py 100.00% <0.00%> (ø)
dipy/viz/app.py 77.25% <0.00%> (+0.50%) ⬆️

@skoudoro
Copy link
Member

Hi @gabknight,

Thank you for this update. If this naming conventions change is really necessary, we will need to set up a deprecation cycle for at least 2 releases and keep the old system (release 1.0.0 was an exception for breaking without any deprecation cycle)
So we need:

  • to keep the old file and create a new one
  • In the old file, add a deprecation warning and call the new one.

Let me know if you need help with that and I will create a PR on local branch/dipy repo.

@skoudoro skoudoro added this to the 1.2 milestone Jan 13, 2020
@gabknight
Copy link
Contributor Author

gabknight commented Feb 9, 2020

Hi @skoudoro, thanks for your response. I wasn't sure how to do the deprecation for a class. Let me know if you would do it differently. Maybe instead of defining a function, I should just do
EuDXDirectionGetter = dipy.reconst.eudx_direction_getter.EuDXDirectionGetter ?

The warning is in peak_direction_getter .py rather than .pyx. Do you think this is an issue?

@skoudoro
Copy link
Member

Hi @gabknight,

I think it is good the way you do it!

It is just missing a small test to make sure that the warning popup.

Furthermore, I need to test it quickly. Thank you !

@gabknight
Copy link
Contributor Author

@skoudoro, Thanks I added the test.

Do you know why those build failed?

@skoudoro
Copy link
Member

skoudoro commented Feb 18, 2020

Not yet, needs to look into it.

  • it seems that something change in Nibabel and their Numpy minimum version is different from ours.
  • Or maybe, they just dropped python3.5

Anyway, I need to look.

arokem added a commit to arokem/dipy that referenced this pull request Feb 20, 2020
This should help with this: dipy#2033 (comment). It seems that nibabel has upped their minimal requirement for 3.0
 and we can follow along.
Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can rebase this PR @gabknight, error on Travis has been fixed.

As I said below, There is one more deprecation to do and this PR will be ready to go. Thank you

@@ -56,7 +56,7 @@ cdef int closest_peak(np.ndarray[np.float_t, ndim=2] peak_dirs,
return 0
return 1

cdef class BaseDirectionGetter(DirectionGetter):
cdef class BasePmfDirectionGetter(DirectionGetter):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BaseDirectionGetter needs a deprecation cycle as you did for EuDXDirectionGetter.

Can you add this deprecation and a small test for it?

Thank you

@skoudoro
Copy link
Member

Sorry for the delay but thank you @gabknight! Everything looks good and you can continue with #2036. merging

@skoudoro skoudoro merged commit a54fcec into dipy:master Apr 15, 2020
@gabknight gabknight deleted the RF_directionGetters branch December 8, 2023 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants