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: Use new name for this function. #2348

Merged
merged 4 commits into from
Apr 9, 2021

Conversation

arokem
Copy link
Contributor

@arokem arokem commented Apr 3, 2021

This shouid eliminate warnings that now routinely appear

@pep8speaks
Copy link

pep8speaks commented Apr 3, 2021

Hello @arokem, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2021-04-08 05:53:22 UTC

@skoudoro skoudoro added this to the 1.5 milestone Apr 3, 2021
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.

Hi @arokem,

Thank you for this update!

it seems you are missing 2-3 real_sph_harm in test_shm.py. Also, you will have to update test_default_lambda_csdmodel.

Thanks !

@arokem
Copy link
Contributor Author

arokem commented Apr 4, 2021

Thanks for taking a look! I also took the opportunity to replace real_sym_sh_basis with real_sh_descoteaux.

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.

Hi @arokem,

You forgot to address the problem with test_default_lambda_csdmodel. You just need to update the number of warnings to be caught.

After that, it should be ready to be in.

Thanks!

@arokem
Copy link
Contributor Author

arokem commented Apr 8, 2021

OK - I think I got it all now. Thanks for the quick review!

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.

All Green, CI's Happy, I will merge this if there is no additional comment by the end of the day.

Thank you for this @arokem!

@skoudoro skoudoro merged commit e96c440 into dipy:master Apr 9, 2021
@jhlegarreta
Copy link
Contributor

@arokem By changing the method name in here:
343f811#diff-bcbfa34194cf36d07616911bb135804418eb5903801f5c0f2d65f24a88c6ada1R125

a duplication was introduced with an existing method name:
https://github.com/dipy/dipy/blob/master/dipy/reconst/tests/test_shm.py#L164

Thus, only the second one is being tested:
https://github.com/dipy/dipy/blob/master/dipy/reconst/tests/test_shm.py#L125
https://github.com/dipy/dipy/blob/master/dipy/reconst/tests/test_shm.py#L164

If the first one, which is calling the deprecated real_sym_sh_mrtrix method is renamed and kept, the deprecation message should be dealt with appropriately (cross-referencing PR #2478).

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.

4 participants