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

Resolve matplotlib deprecation issue #74

Merged
merged 3 commits into from
Jul 19, 2024
Merged

Conversation

afni-dglen
Copy link
Contributor

Closes #
#66 (comment)

Couldn't get rebase to work, so reforked again and put the changes for editor.py and setup.cfg requested in that previous issue.

Proposed Changes

Implement changes for deprecated rectprops with props in matplotlib function call. This version checks for older versions of matplotlib to continue with old behavior.
Also removed some of the constraints on matplotlib in setup.cfg, but kept requirement for some version, consistent with physiobids setup.cfg.

Change Type

  • bugfix (+0.0.1)
  • minor (+0.1.0)
  • major (+1.0.0)
  • refactoring (no version update)
  • test (no version update)
  • infrastructure (no version update)
  • documentation (no version update)
  • other

Checklist before review

  • I added everything I wanted to add to this PR.
  • [Code or tests only] I wrote/updated the necessary docstrings.
  • [Code or tests only] I ran and passed tests locally.
  • [Documentation only] I built the docs locally.
  • My contribution is harmonious with the rest of the code: I'm not introducing repetitions.
  • My code respects the adopted style, especially linting conventions.
  • The title of this PR is explanatory on its own, enough to be understood as part of a changelog.
  • I added or indicated the right labels.
  • I added information regarding the timeline of completion for this PR.
  • Please, comment on my PR while it's a draft and give me feedback on the development!

@afni-dglen afni-dglen mentioned this pull request Jul 18, 2024
@maestroque maestroque self-requested a review July 19, 2024 12:36
Copy link
Contributor

@maestroque maestroque left a comment

Choose a reason for hiding this comment

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

Currently, the pre-commit CI test is failing. You should have this installed in your local repository first, so that the pre-commit hooks run locally, only allowing to commit once the checks pass.

# Given the virtual environment you are working on is activated
pip install pre-commit
pre-commit install
# Now, each time you commit the pre-commit hooks will run only allowing for the commit once all of the tests pass

# Run the following command to run the test prior to commiting for a sanity check
pre-commit run --all-files

Now you would need to run the pre-commit hooks to correct some styling issues and then re-add the files and commit. Otherwise you will get a warning for unstaged files altered by pre-commit

@afni-dglen

@@ -60,29 +60,36 @@ def __init__(self, data):
delete = functools.partial(self.on_edit, method="delete")
reject = functools.partial(self.on_edit, method="reject")
insert = functools.partial(self.on_edit, method="insert")

# Check matplotlib version rectprops is deprecated with matplotlib 3.5.0 and then obsolete
if matplotlib.__version__ >= '3.5.0':
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to explicitly import matplotlib for this. The current imports don't cover this. This is also indicated in the pre-commit fail

Copy link
Contributor

Choose a reason for hiding this comment

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

This comparison does not work consistently. E.g. from my ipython session

In [17]: matplotlib.__version__
Out[17]: '3.9.1'

In [18]: matplotlib.__version__ > '3.13.0'
Out[18]: True

In [19]: matplotlib.__version__ > '3.5.0'
Out[19]: True

It should be handle by a module such as packaging.version or importlib
@smoia

@maestroque maestroque changed the title Refork to add matplotlib deprecation issue Resolve matplotlib deprecation issue Jul 19, 2024
@maestroque maestroque added MInormod-breaking For development only, this PR increments the minor version (0.+1.0) but breaks compatibility Minormod This PR generally closes an "Enhancement" issue. It increments the minor version (0.+1.0) and removed MInormod-breaking For development only, this PR increments the minor version (0.+1.0) but breaks compatibility labels Jul 19, 2024
@github-actions github-actions bot added the Internal Changes affect the internal API. It doesn't increase the version, but produces a changelog label Jul 19, 2024
Copy link
Contributor

@maestroque maestroque left a comment

Choose a reason for hiding this comment

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

Looks good to me 🚀

Thanks @afni-dglen

@maestroque maestroque merged commit 8fc9c1d into physiopy:master Jul 19, 2024
2 checks passed
@smoia
Copy link
Member

smoia commented Jul 19, 2024

🚀 PR was released in 0.5.0 🚀

@smoia smoia added the released This issue/pull request has been released label Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Changes affect the internal API. It doesn't increase the version, but produces a changelog Minormod This PR generally closes an "Enhancement" issue. It increments the minor version (0.+1.0) released This issue/pull request has been released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants