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

PR: Implement small plotting API improvements. #1209

Merged
merged 4 commits into from
Oct 20, 2023
Merged

Conversation

tjdcs
Copy link
Contributor

@tjdcs tjdcs commented Oct 19, 2023

No description provided.

@coveralls
Copy link

coveralls commented Oct 19, 2023

Coverage Status

coverage: 99.798%. remained the same when pulling f8dd81e on feature/tjdcs/v0.4.4 into 272c010 on develop.

Copy link
Member

@KelSolaar KelSolaar left a comment

Choose a reason for hiding this comment

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

LGTM besides the imports reorder! See my comment above.

axes = cast(Axes, axes)
figure = axes.figure

while isinstance(figure, SubFigure):
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using while here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I first read the docs I thought the get figure might sometimes return a parent subfigure

Comment on lines 30 to 43
import functools
import itertools
from dataclasses import dataclass, field
from functools import partial

import matplotlib.cm
import matplotlib.pyplot as plt
import matplotlib.ticker
import numpy as np
from cycler import cycler
from dataclasses import dataclass, field
from functools import partial
from matplotlib.axes import Axes
from matplotlib.colors import LinearSegmentedColormap
from matplotlib.figure import Figure
from matplotlib.patches import Patch
from mpl_toolkits.mplot3d.axes3d import Axes3D
Copy link
Member

Choose a reason for hiding this comment

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

Let's try to not reorder the imports automatically, it looks like isort work here and it is not doing a great job. We follow PEP8: https://peps.python.org/pep-0008/#imports

Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.
Imports should be grouped in the following order:

Standard library imports.
Related third party imports.
Local application/library specific imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it became clear to me that this was a way bigger tool / dev change than I was thinking originally. Removed in this PR.

But I want to revisit this in the future. It's reading - pleasing when my auto-completed imports get put in the correct place by a tool. Ruff has some capability to do this (but I'm not sure about configurability) and isort can probably be made to match pep8.

@@ -701,7 +711,7 @@ def render(
figure.savefig(settings.filename)

if settings.show:
plt.show()
plt.show(block=settings.block)
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@tjdcs
Copy link
Contributor Author

tjdcs commented Oct 20, 2023

Addressed all comments @KelSolaar

@KelSolaar
Copy link
Member

PR is still draft @tjdcs, not sure if it is intended!

@tjdcs tjdcs marked this pull request as ready for review October 20, 2023 22:51
@KelSolaar KelSolaar merged commit 87d322c into develop Oct 20, 2023
26 checks passed
@KelSolaar KelSolaar changed the title Small plotting features PR: Implement small plotting API improvements. Oct 20, 2023
@KelSolaar
Copy link
Member

/merged

@KelSolaar KelSolaar deleted the feature/tjdcs/v0.4.4 branch October 20, 2023 22:53
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.

None yet

3 participants