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

Dependency updates #9

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jrdalenberg
Copy link

I was doing a power analysis for myself and noticed that the code was not working with the latest packages.

These updates work for me using Python 3.9.18.

I updated:

  • appending pandas dataframes with new dataframes
  • Nibabel data extraction
  • Mask calculation by using math_img from Nilearn.
  • Matplotlib normed argument.
  • The example notebook

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Apr 2, 2024

Hey @jrdalenberg

Thanks for the PR.

Need to double check but:

  • plotting seems only used in the notebook so not sure it is worth adding a dependency to the package
  • similarly I wonder if we need to have all of nilearn as a dependency for this much

Making sure that the demo notebook can run at any time is VERY good point though so it should probably be part of the CI.

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Apr 2, 2024

Also it seems that the changes are breaking some tests on Windows only.

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Apr 2, 2024

Also it seems that the changes are breaking some tests on Windows only.

my bad the tests are failing on main too, so this is not related to the PR. Sorry.

@jrdalenberg
Copy link
Author

Hey @jrdalenberg

Thanks for the PR.

Need to double check but:

* plotting seems only used in the notebook so not sure it is worth adding a dependency to the package

* similarly I wonder if we need to have all of nilearn as a dependency for this much

Making sure that the demo notebook can run at any time is VERY good point though so it should probably be part of the CI.

Thanks for the responses.

@ Plotting: matplotlib was already in there. The line was only updated with a comma. The plotting features in core code used an outdated argument.

@ Masking using nilearn: I understand your point. If I find some time I could implement just the masking logic.

Should I wait on a fix for the windows tests?

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Apr 2, 2024

@ Plotting: matplotlib was already in there. The line was only updated with a comma. The plotting features in core code used an outdated argument.

And that's why I should not review things too early in the morning!!! 🙈

@ Masking using nilearn: I understand your point. If I find some time I could implement just the masking logic.

If you have time that'd be great

Should I wait on a fix for the windows tests?
nope: keep going without a fix for those

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Apr 2, 2024

FYI I have just updated the code formatting tooling.

To get rid of the annoying CI failure you can:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants