Examples in doc/examples/applications edited using object-oriented Matplotlib style #7346
Conversation
lagru
added
the
📄 type: Documentation
There was a problem hiding this comment.
Thanks very much for starting the clean-up @GParolini! Though, I think you missed a few cases where matplotlib's old API is still in use. You can find these cases like so
$ grep -R "plt.plot" --include '*.py' doc/
doc/examples/applications/plot_image_comparison.py:plt.plot()
doc/examples/applications/plot_image_comparison.py:plt.plot()
doc/examples/applications/plot_image_comparison.py:plt.plot()
doc/examples/transform/plot_radon_transform.py: plt.plot(response, label=f)
doc/examples/segmentation/plot_rolling_ball.py:plt.plot(x, label='original')
doc/examples/segmentation/plot_rolling_ball.py:plt.plot(x - background, label='radius=80')
doc/examples/segmentation/plot_rolling_ball.py:plt.plot(x - background2, label='radius=10')
doc/examples/segmentation/plot_hausdorff_distance.py:plt.plot(set_ax, set_ay, 'or')
doc/examples/segmentation/plot_hausdorff_distance.py:plt.plot(set_bx, set_by, 'og')
doc/examples/segmentation/plot_hausdorff_distance.py:plt.plot(x_line, y_line, 'y')
doc/examples/segmentation/plot_hausdorff_distance.py:plt.plot(x_line, y_line, 'y')
doc/source/auto_examples/applications/plot_image_comparison.py:plt.plot()
doc/source/auto_examples/applications/plot_image_comparison.py:plt.plot()
doc/source/auto_examples/applications/plot_image_comparison.py:plt.plot()
doc/source/auto_examples/transform/plot_radon_transform.py: plt.plot(response, label=f)
doc/source/auto_examples/segmentation/plot_rolling_ball.py:plt.plot(x, label='original')
doc/source/auto_examples/segmentation/plot_rolling_ball.py:plt.plot(x - background, label='radius=80')
doc/source/auto_examples/segmentation/plot_rolling_ball.py:plt.plot(x - background2, label='radius=10')
doc/source/auto_examples/segmentation/plot_hausdorff_distance.py:plt.plot(set_ax, set_ay, 'or')
doc/source/auto_examples/segmentation/plot_hausdorff_distance.py:plt.plot(set_bx, set_by, 'og')
doc/source/auto_examples/segmentation/plot_hausdorff_distance.py:plt.plot(x_line, y_line, 'y')
doc/source/auto_examples/segmentation/plot_hausdorff_distance.py:plt.plot(x_line, y_line, 'y')I'd approve the current state except for a few minor things as many changes are a clear improvement already. :D
| @@ -94,7 +94,7 @@ | |||
|
|
|||
| def show_plane(ax, plane, cmap="gray", title=None): | |||
| ax.imshow(plane, cmap=cmap) | |||
| ax.axis("off") | |||
| ax.set_axis_off() | |||
There was a problem hiding this comment.
I'm curious why you changed some of these but not all? E.g. in plot_general.py?
To be clear I don't mind either way, both methods are part of the OO API. But it does blow up the diff quite a lot. Keeping diffs small can help speeding things along as getting big PRs merged can be a slog sometimes.
The same thing applies with
fig, (ax1, ax2) = plt.subplots(...)andfig, axes = plt.subplots(...)
which were changed to fig, ax = plt.subplots(...).
There was a problem hiding this comment.
I have not made yet to the file plot_general.py. I only changed the files in the folder applications/ so far.
I left (ax1, ax2) and similar in a few instances where I feared to break the docs making a change. E.g. in plot_image_comparison.py, GridSpec is used to arrange the plots in a more complex layout and changing fig, (ax1, ax2, ax3) into fig, ax generated an error I could not solve.
There was a problem hiding this comment.
I don't see any reason, in general, for changing fig, axes = plt.subplots(...) or fig, (ax1, ax2) = plt.subplots(...) into fig, ax = plt.subplots(...). Using ax1 and ax2 may even be more convenient and readable than ax[0] and ax[1]. Anyway, these changes have nothing to do with the OO style vs pyplot-style issue.
There was a problem hiding this comment.
I only changed the files in the folder applications/ so far.
Ah I should have noticed. Then let's keep this PR focused on only that directory. And we can do follow-ups for the other directories.
I concur with @mkcor but how do we go forward? I'd say to minimize work on all sides let's keep the instances where you changed .axis("off") to .set_axis_off() but let's not change more. They are easy to review but the consistency is not really a big improvement in this case.
For the (ax1, ax2) vs ax issue, I also don't see a big difference. Let's keep the ones you changed and not change more going forward. @mkcor agreed?
There was a problem hiding this comment.
Sorry, now I am bit lost about the changes that I still need to do for this pull request and the general guidelines you wish to see implemented. I try to recap:
For this pull request
- Change plt.close() in fig.close()
- Change plt.tight_layout() in fig.tight_layout()
- I can revert the set_axis_off() in axis("off") when this was the original use (just say what you prefer)
- I think I already reverted all fig, ax = plt.subplots() in _, ax = plt.subplots(), but if I will check if I missed any.
In general
- _, ax = plt.subplots() does not need to be changed in fig, ax = plt.subplots()
- fig, axes = plt.subplots() does not need to be changed in fig, ax = plt.subplots()
- fig, (ax1, ax2) = plt.subplots() and similar do not need to be changed in fig, ax = plt.subplots()
- Axis notations ax1, etc. and ax[1], etc. are both acceptable
- set_axis_off() and axis("off") are both acceptable
- plt.close() should be changed in fig.close()
- plt.tight_layout() should be changed in fig.tight_layout()
- plt.plot() must be replaced by plt.show()
- Editorial changes in docs (capital letters in titles, etc.) are better left for future interventions.
Please provide your feedback.
There was a problem hiding this comment.
Hi both,
@GParolini said (private communication) that she would away in the next days and we could take over the PR. For future reference:
let's keep the instances where you changed .axis("off") to .set_axis_off() but let's not change more
Agreed.
For the
(ax1, ax2)vsaxissue, I also don't see a big difference. Let's keep the ones you changed and not change more going forward. @mkcor agreed?
Hmm, I would revert these, since a) it's unrelated to the OO vs pyplot-style and b) (ax1, ax2) is correct. I can take care of it (see above).
Change plt.close() to fig.close()
Yes, for sure, since plt.close() is pyplot-style, so it corresponds to the issue we're addressing with this PR.
Change plt.tight_layout() in fig.tight_layout()
idem
I don't think I need to go through all the bullet points, since the issue is unambiguous: Everything pyplot-style needs to be changed into OO style, period.
There was a problem hiding this comment.
Oh, maybe this one is worth going in more detail:
plt.plot() must be replaced by plt.show()
plt.show() might be pyplot-style(?) but it's convenient to call it at the very end of the file, so that CLI users can run, say, $ python doc/examples/applications/plot_coins_segmentation.py and have all the figures which show and stay (do not close).
| @@ -94,7 +94,7 @@ | |||
|
|
|||
| def show_plane(ax, plane, cmap="gray", title=None): | |||
| ax.imshow(plane, cmap=cmap) | |||
| ax.axis("off") | |||
| ax.set_axis_off() | |||
There was a problem hiding this comment.
I don't see any reason, in general, for changing fig, axes = plt.subplots(...) or fig, (ax1, ax2) = plt.subplots(...) into fig, ax = plt.subplots(...). Using ax1 and ax2 may even be more convenient and readable than ax[0] and ax[1]. Anyway, these changes have nothing to do with the OO style vs pyplot-style issue.
| @@ -166,7 +166,7 @@ def plot_comparison(plot1, plot2, title1, title2): | |||
|
|
|||
| footprint = ski.morphology.diamond(3) | |||
| mask_open = ski.morphology.opening(mask_2, footprint) | |||
| plot_comparison(mask_2, mask_open, "mask before", "after opening") | |||
| plot_comparison(mask_2, mask_open, "Mask before", "After opening") | |||
There was a problem hiding this comment.
Why not, but this is unrelated to the current issue, and the PR is very long already, so I would prefer having these changes in a separate/dedicated PR. Thanks!
There was a problem hiding this comment.
Reverted. In general, though, it would be helpful to know if plot titles should start with a capital letter or not. At present, there is a mix, even within the same example.
There was a problem hiding this comment.
You are absolutely right, @GParolini. It's just that, precisely, this issue deserves its own... issue/PR 😉
(see also @lagru's comment about avoiding blowing up the diff 🙏)
| false ones usually have single detection. First algorithm searches for | ||
| clusters: two rectangle detections are placed in the same cluster if the | ||
| intersection score between them is larger then | ||
| ``intersection_score_threshold``. The intersection score is computed using the | ||
| equation (intersection area) / (small rectangle ratio). The described | ||
| intersection criteria was chosen over intersection over union to avoid a corner | ||
| case when small rectangle inside of a big one have small intersection score. | ||
| case when small rectangle inside of a big one has small intersection score. |
There was a problem hiding this comment.
This gallery example would definitely need some proofreading... (see also "criteria was chosen" above) This would deserve its own issue/PR as well.
doc/examples/applications/plot_fluorescence_nuclear_envelope.py
Outdated
Show resolved
Hide resolved
| @@ -76,10 +76,10 @@ | |||
| fig, ax = plt.subplots(ncols=2, figsize=(10, 5)) | |||
| ax[0].imshow(image) | |||
| ax[0].set_title('Original') | |||
| ax[0].axis('off') | |||
| ax[0].set_axis_off() | |||
There was a problem hiding this comment.
I'd like to ask the same question @lagru did in #7346 (comment). Maybe because set_axis_off() looks more like set_title() and the other set_ functions (so, visually and mentally, it's nicer/lighter/smoother)? @GParolini I'd be curious if you had other ideas on this!
There was a problem hiding this comment.
Both axis("off") and set_axis_off() were used. I always changed axis("off") in set_axis_off() because all the other axis methods (title, xlim, etc.) use set. It seemed to me more natural to have the same structure for everything.
|
|
Thanks for helping us make the docs more consistent, @GParolini! |
Co-authored-by: Lars Grüter <lagru+github@mailbox.org>
Co-authored-by: Lars Grüter <lagru+github@mailbox.org>
This reverts commit 09df1bc.
The one linked in #7346 (comment). The text is missing from the image. Also axes isn't needed in the example. Feel welcome to use |
Oh, thank you! Somehow I missed that comment.
I see. I'll sleep on this and wrap up tomorrow. 🙏 |
|
PS: @lagru thank you for double-checking the example visually ❤️ |
|
Thanks @GParolini! :D |
Description
Issue #7330
Edited files in doc/examples/applications.
Checklist
./doc/examplesfor new featuresRelease note
We use changelist to
compile each pull request into an item of the release notes. Please refer to
the instructions
and past release notes
for guidance and examples.