Introducing astro_support, to enable time_support and quantity_support simultaneously #17575
Introducing astro_support, to enable time_support and quantity_support simultaneously #17575spranav1205 wants to merge 24 commits into
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
There was a problem hiding this comment.
Welcome to Astropy 👋 and congratulations on your first pull request! 🎉
A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.
If you feel that this pull request has not been responded to in a timely manner, please send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail feedback@astropy.org.
neutrinoceros
left a comment
There was a problem hiding this comment.
Thanks for opening this, it seems like a good start !
A couple remarks at this point
- the decorator name, as was originally proposed in #8860 was
astropy_support. I think we should retain it - I think the new decorator should have its own module1
- we'll need to add actual image comparison tests
Footnotes
-
the point being that I'd rather not have it borrow
visualization.units. An argument could be made that all three decorators could be moved to a common module but that is beyond the scope of this PR. ↩
|
Another remark is that both def astropy_support(*, quantity_support_kwargs=None, time_support_kwargs=None):
with ExitStack as stack:
stack.enter_context(quantity_support(**(quantity_support_kwargs or {}))
stack.enter_context(time_support(**(time_support_kwargs or {}))
yield |
|
I have made the changes. Before I commit, should I make a separate module for astropy_support? As for the test case, I was thinking we could generate an images like this- Within astropy_support, quantity_support takes care of the m / km scaling and time_support will take care of the limits of the graph (this is similar to time_support tests). Thus we get two coincident graphs as follows. As quantity_support and time_support are already tested extensively, one test should suffice for this right? |
Yes, I think that's what makes the most sense. Also, I want to mention that git branch history can be rewritten after the fact and we can help with it, so don't feel like you have to get things right on every single commit !
yes, this sounds reasonable to me ! |
|
Yes okay, I'll keep that in mind :) |
| Examples | ||
| ---------- | ||
|
|
||
| >>> from astropy.visualization.units import astro_support | ||
| >>> with astro_support(): | ||
| ... plt.figure() | ||
| ... plt.plot([1, 2, 3] * u.m) | ||
| ... plt.plot(Time(['2000-01-01', '2000-01-02', '2000-01-03']).plot_date) | ||
| ... plt.draw() | ||
|
|
||
| >>> @astro_support() | ||
| ... def plot_example(): | ||
| ... plt.figure() | ||
| ... plt.plot([1, 2, 3] * u.m) | ||
| ... plt.plot(Time(['2000-01-01', '2000-01-02', '2000-01-03']).plot_date) | ||
| ... plt.draw() | ||
| ... plt.show() | ||
|
|
||
| >>> plot_example() |
| ax.coords[0].set_major_formatter(double_format) | ||
| ax.coords[1].set_major_formatter(fruit_format) | ||
| return fig | ||
|
|
There was a problem hiding this comment.
Please use double backticks here as we do not want any "live" links in the change log to prevent future broken links. We try to not have to go back and fix up old change log.
| ax.coords[1].set_major_formatter(fruit_format) | ||
| return fig | ||
|
|
||
| @figure_test(tolerance=1) |
There was a problem hiding this comment.
Please see https://docs.astropy.org/en/latest/development/testguide.html#image-tests-with-pytest-mpl for how to set up the "truth" for this new image test. Thanks!
| Enable support for plotting `astropy.units.Quantity` and `astropy.time.Time` instances in | ||
| matplotlib. | ||
|
|
||
| It can be used as a decorator or with a ``with`` statement. |
There was a problem hiding this comment.
nit, but I'd like the examples to be presented in the same order they are introduced in this sentence.
(out of scope here, but I'm also realizing that quantity_support and time_support's respective docstrings currently do not demonstrate the decorator style, although they both support it)
There was a problem hiding this comment.
Yes, that makes sense. I've made the correction.
Maybe after this PR, another issue can be opened to fix quantity_support and time_support, and have all three in the same module.
There was a problem hiding this comment.
I thinkg that moving code around is rarely worth the cost of making git blame less useful. However, +1 on doing a follow up to complete docstrings !
|
I made the formatting changes. |
|
Re: hash -- please see https://docs.astropy.org/en/latest/development/testguide.html#new-hash-libraries |
updated json with new hash
|
I used |
|
Thank you ! We still need a couple minor adjustments to get all green CI:
|
|
@neutrinoceros Now only the new image test CI is left. Once that's sorted the PR should be good to go right? |
Correcting file path Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
as far as I'm concerned yes, but I'd like someone else to approve too before this goes in. Thanks for your patience and dedication ! Seeing that the test is still failing, even though you added the hash, I'm wondering: did you generate it locally ? We need to use exact hashes as they come out from Circle CI because matplotlib image generation is not pixel-to-pixel portable, so we can only compare hashes from the same exact machine that runs the image tests. |
|
The |
neutrinoceros
left a comment
There was a problem hiding this comment.
Ah ! I see, I somehow forgot that we had two of these. Well, good to go as far as I'm concerned. Will give a couple days for @pllim to comment again if she wants to. Thank you for your contribution !
|
Given the holidays, might take me a few more days to get to this. Thanks for your patience. |
| "astropy.visualization.wcsaxes.tests.test_wcsapi.test_wcsapi_5d_with_names": "c90ae6f3b0f9f407ca7a866fa648dc3ef4bea78369315292bc82f16104ed5736", | ||
| "astropy.visualization.wcsaxes.tests.test_wcsapi.test_wcsapi_2d_celestial_arcsec": "4b743d645a85d7516decbcf4a831c127af5a1800072597c2a1299d17fb186adb" | ||
| "astropy.visualization.wcsaxes.tests.test_wcsapi.test_wcsapi_2d_celestial_arcsec": "4b743d645a85d7516decbcf4a831c127af5a1800072597c2a1299d17fb186adb", | ||
| "astropy.visualization.wcsaxes.tests.test_images.test_astropy_support": "aee3dd8cfc0998ec2d904549c6527fa55f3b16cbe9a30c222a2c0ac6a6db155e" |
There was a problem hiding this comment.
For consistency with existing entries, I would still like this to be moved higher up (where other astropy.visualization.wcsaxes.tests.test_images entries are), ordered alphabetically if possible.
| "astropy.visualization.wcsaxes.tests.test_wcsapi.test_wcsapi_5d_with_names": "c90ae6f3b0f9f407ca7a866fa648dc3ef4bea78369315292bc82f16104ed5736", | ||
| "astropy.visualization.wcsaxes.tests.test_wcsapi.test_wcsapi_2d_celestial_arcsec": "4b743d645a85d7516decbcf4a831c127af5a1800072597c2a1299d17fb186adb" | ||
| "astropy.visualization.wcsaxes.tests.test_wcsapi.test_wcsapi_2d_celestial_arcsec": "4b743d645a85d7516decbcf4a831c127af5a1800072597c2a1299d17fb186adb", | ||
| "astropy.visualization.wcsaxes.tests.test_images.test_astropy_support": "aee3dd8cfc0998ec2d904549c6527fa55f3b16cbe9a30c222a2c0ac6a6db155e" |
There was a problem hiding this comment.
Same comment as https://github.com/astropy/astropy/pull/17575/files#r1904638767
There was a problem hiding this comment.
I cannot find any mention of this new module in https://astropy--17575.org.readthedocs.build/en/17575/visualization/ref_api.html , so none of this docstring is actually being rendered.
You might need to modify https://github.com/astropy/astropy/blob/main/astropy/visualization/__init__.py to fix this. Therefore, defining __all__ in this module is also very important, which I do not see here.
pllim
left a comment
There was a problem hiding this comment.
Thanks! This is close, but I am going to block merge until we can be sure the new module is being documented properly.
Would also be nice if subpackage maintainers (@larrybradley @Cadair @astrofrog) could review.
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
astrofrog
left a comment
There was a problem hiding this comment.
This seems ok to me in theory but I don't like the name astropy_support as it doesn't convey what about astropy is supported. Having e.g. astropy_types_support or similar would at least be more explicit - what do others think?
|
Yes, I agree. Astropy support should be reserved for broader application anyway... |
|
I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks! If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here. |

Description
This pull request is to fix #8860
We introduce
astro_support, a context manager, that enables bothquantity_supportandtime_support.