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

Version checks for matplotlib [unitaryHACK] #1556

Merged
merged 9 commits into from
May 17, 2021

Conversation

Yash-10
Copy link
Contributor

@Yash-10 Yash-10 commented May 13, 2021

Description
This PR attempts to add version checks for resolving deprecation warnings for matplotlib in the bloch and visualization modules. Specifically, it attempts to resolve warnings related to matplotlib>=3.4 and matplotlib>=3.3.

Places where the change has been made:

  • proj3d.proj_transform
  • Axes3D

Thanks!

Related issues or PRs
Fix #1503 and #1502.

Changelog
Added a version checking condition to handle specific functionalities depending on the matplotlib version.

@Yash-10 Yash-10 changed the title Version tests matplotlib Version tests for matplotlib [unitaryHACK] May 13, 2021
@Yash-10 Yash-10 changed the title Version tests for matplotlib [unitaryHACK] Version checks for matplotlib [unitaryHACK] May 13, 2021
Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

Looking good! I left some suggestions for tidying it up a little (not quite sure how tidy it can be).

qutip/bloch.py Outdated Show resolved Hide resolved
qutip/bloch.py Outdated Show resolved Hide resolved
qutip/bloch.py Outdated
Comment on lines 483 to 484
else:
pass
Copy link
Member

Choose a reason for hiding this comment

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

else: pass is unnecessary - it's fine to just have the if ...: part of the statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@Yash-10 Yash-10 force-pushed the Version-tests-matplotlib branch 2 times, most recently from 05a77e4 to 3fe9183 Compare May 14, 2021 12:53
@coveralls
Copy link

coveralls commented May 14, 2021

Coverage Status

Coverage decreased (-0.006%) to 64.896% when pulling 8466e01 on Yash-10:Version-tests-matplotlib into c1a3b9d on qutip:master.

qutip/bloch.py Outdated
import matplotlib.pyplot as plt
from mpl_toolkits.mplot3d import Axes3D
from matplotlib.patches import FancyArrowPatch
from mpl_toolkits.mplot3d import proj3d

def matplotlib_version_gte(version='3.4'):
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't have a default value here at all - it's not obvious why it's 3.4 (somebody two months in the future will have no idea), so it's not clear what matplotlib_version_gte() will do. Better to make all the calls matplotlib_version_gte('3.4') or so.

(a minor style note that isn't important at all: "private" functions that aren't meant to be exported usually start with an underscore _ in Python convention. It matters more if a module doesn't specify an __all__ metadata object, but in QuTiP we always do.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(a minor style note that isn't important at all: "private" functions that aren't meant to be exported usually start with an underscore _ in Python convention. It matters more if a module doesn't specify an all metadata object, but in QuTiP we always do.)

Thanks, I will keep that in mind!

qutip/bloch.py Outdated
Comment on lines 58 to 61
if parse_version(matplotlib.__version__) >= parse_version(version):
return True
else:
return False
Copy link
Member

Choose a reason for hiding this comment

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

if x: return True; else: return False is unnecessary - you can just return x, e.g.

return parse_version(matplotlib.__version__) >= parse_version(version)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! I should have done that before! Thank you for the suggestion!

@jakelishman jakelishman added this to the QuTiP 4.6.2 milestone May 14, 2021
@Yash-10
Copy link
Contributor Author

Yash-10 commented May 14, 2021

Thank you @jakelishman and @hodgestar for your suggestions throughout the PR!

At this stage, the following things are added:

  • Getting rid of the previously implemented matplotlib_version_gte method. Instead following @jakelishman 's advice to factor version checking once at the top and use it throughout the module. This prevents any version checking in the user guide since the axes3D_ method can be directly imported.
  • Since axes.M seems to work even for matplotlib==3.0, converted all instances of renderer.M to axes.M to keep version checking at the minimum.

Thanks!!

Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

Left one more comment, otherwise looks good to me.

@Yash-10 Yash-10 force-pushed the Version-tests-matplotlib branch 3 times, most recently from 073034e to 9c43dfc Compare May 15, 2021 09:30
Copy link
Contributor

@hodgestar hodgestar 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! Once @jakelishman is happy too, we can merge.

qutip/bloch.py Outdated
Comment on lines 55 to 56
def _axes3D(*args, **kwargs):
fig = args[0]
Copy link
Member

Choose a reason for hiding this comment

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

It's better to spell this as

def _axes3D(fig, *args, **kwargs):
    ...

because then if there's an error calling the function (i.e. by passing no arguments), you get the "normal" Python error

TypeError: _axes3D() missing 1 required positional argument: 'fig'

What I wrote is a pretty common Python idiom when you want to pick out the first positional argument (or more). You can similarly pick out special keyword arguments with def xxx(*args, out=None, linewidth=None, **kwargs).

It also would be good if you could write a little comment above the if parse_version() line (just a one- or two-line comment) explaining why this is necessary; we understand right now why it was put in place, but the next people to look at the code won't have the context of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

(and a similar comment at the visualisation.py change as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jakelishman for all these suggestions! I have made the necessary changes.

@jakelishman
Copy link
Member

This is pretty much ready to go for me, too. I added one more thing, mostly asking for a code comment to explain why we're doing this slightly unusual thing - we've always got to be thinking about the people who'll come after us to maintain the code, too.

Thank you very much for going through so many rounds of this.

Add condition for docs change

Add imports in user guide
Use mpl instead of matplotlib
Remove small errors

Remove matplotlib import in user guide

Abide by private function variable names

restore blit argument value
@jakelishman jakelishman linked an issue May 16, 2021 that may be closed by this pull request
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

If you click through to the failing test and scroll through the logs, it's reporting a bug in visualization.py, which needs fixing before this can be merged.

@Yash-10
Copy link
Contributor Author

Yash-10 commented May 17, 2021

Ah, just needed to change the position of the import statements in visualization.py. It is fixed now. Thanks for all your suggestions!

I think it is good to go now @jakelishman @hodgestar ?

Comment on lines 375 to 387
ax = Axes3D(fig)
ax = _axes3D(fig, azim=-35, elev=35)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to change the arguments in this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for missing all these intricacies! There was absolutely no reason for me to change that. Thanks again.

Copy link
Member

Choose a reason for hiding this comment

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

That's no problem, it happens to everyone when we go back and forth between changes. I think this is ready now, but we'll wait til the tests have officially passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, absolutely!

@hodgestar hodgestar merged commit b21966b into qutip:master May 17, 2021
quantshah pushed a commit to quantshah/qutip that referenced this pull request Jun 2, 2021
Version checks for matplotlib [unitaryHACK]
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.

matplotlib 3.4 issues new deprecation warnings Bloch sphere requires matplotlib >= 3.3
4 participants