Skip to content

Introducing astro_support, to enable time_support and quantity_support simultaneously #17575

Closed
spranav1205 wants to merge 24 commits into
astropy:mainfrom
spranav1205:main
Closed

Introducing astro_support, to enable time_support and quantity_support simultaneously #17575
spranav1205 wants to merge 24 commits into
astropy:mainfrom
spranav1205:main

Conversation

@spranav1205
Copy link
Copy Markdown

@spranav1205 spranav1205 commented Dec 22, 2024

Description

This pull request is to fix #8860

We introduce astro_support, a context manager, that enables both quantity_support and time_support.

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@github-actions
Copy link
Copy Markdown
Contributor

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.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

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

  1. 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.

Comment thread astropy/visualization/units.py Outdated
@neutrinoceros neutrinoceros added this to the v7.1.0 milestone Dec 22, 2024
@neutrinoceros
Copy link
Copy Markdown
Contributor

neutrinoceros commented Dec 22, 2024

Another remark is that both time_support and quantity_support optionally accept arguments. Perharps it'd be good to re-expose them. Probably something like:

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

@spranav1205
Copy link
Copy Markdown
Author

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-

@pytest.mark.mpl_image_compare
def test_astropy_support():
  x_meters = np.linspace(1, 10, 50) * u.m  # Distances in meters
  x_kilometers = np.linspace(0.001, 0.01, 50) * u.km  # Distances in kilometers
  
  y_seconds = Time('2000-01-01T00:10:00', scale='utc') + TimeDelta(np.linspace(0, 3600, 50), format='sec')  
  
  start_time = Time('2000-01-01T00:00:00', scale='utc')
  end_time = start_time + TimeDelta(2/24, format='jd')  
  
  with astropy_support():
    fig, ax = plt.subplots()

    ax.plot(x_meters, y_seconds, label='Meters vs. Seconds')
    ax.plot(x_kilometers, y_seconds, label='Kilometers vs. Seconds')
    
    ax.set_ylim(start_time, Time((end_time), format='jd'))

    ax.set_xlabel('Distance (m)')
    ax.set_ylabel('Time')
    ax.legend()

return fig

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.

image

As quantity_support and time_support are already tested extensively, one test should suffice for this right?

@neutrinoceros
Copy link
Copy Markdown
Contributor

Before I commit, should I make a separate module for astropy_support?

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 !

As quantity_support and time_support are already tested extensively, one test should suffice for this right?

yes, this sounds reasonable to me !

@spranav1205
Copy link
Copy Markdown
Author

Yes okay, I'll keep that in mind :)
I can't think of a name for the module, so I have just named it astropy_support for now. Also, I have added the test case in astropy\visualization\wcsaxes\tests\test_images.py. However, I'm unsure of the path to the baseline images, so I haven't generated the images yet.

Comment on lines +14 to +32
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()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be dedented.

ax.coords[0].set_major_formatter(double_format)
ax.coords[1].set_major_formatter(fruit_format)
return fig

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 !

@spranav1205
Copy link
Copy Markdown
Author

spranav1205 commented Dec 24, 2024

I made the formatting changes.
I am confused about the testing, though. As there is no reference image, tox -e py311-test-image-mpl360-cov is failing, and tox -e py311-test-image-mpl360-cov -- --mpl-generate-hash-library=astropy/tests/figures/py311-test-image-mpl360-cov.json is not generating a new hash either. The documentation says I don't need to generate a reference image and that a reference is generated when the PR is merged.
Do I skip the test? Or is there a way to create the hash map locally? (that includes the new test)

@pllim
Copy link
Copy Markdown
Member

pllim commented Dec 24, 2024

updated json with new hash
@spranav1205
Copy link
Copy Markdown
Author

I used .. plot:: as used in other docstrings, and it works. Is there anything?

@neutrinoceros
Copy link
Copy Markdown
Contributor

Thank you ! We still need a couple minor adjustments to get all green CI:

@spranav1205
Copy link
Copy Markdown
Author

@neutrinoceros Now only the new image test CI is left. Once that's sorted the PR should be good to go right?
I would like to contribute more. Please let me know if there are any issues where I can help. Thank you for your guidance!

Comment thread astropy/tests/figures/py311-test-image-mpl360-cov.json Outdated
spranav1205 and others added 2 commits January 1, 2025 12:10
Correcting file path

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
@neutrinoceros
Copy link
Copy Markdown
Contributor

Once that's sorted the PR should be good to go right?

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.

@spranav1205
Copy link
Copy Markdown
Author

The py311-test-image-mpldev-cov was failing. I updated the JSON file for the same. Now the check is passing. The hash is same as the one generated on CircleCI, thus the tests passed.

Copy link
Copy Markdown
Contributor

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

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 !

@pllim
Copy link
Copy Markdown
Member

pllim commented Jan 2, 2025

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"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

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.

@astrofrog astrofrog modified the milestones: v7.1.0, v7.2.0 Apr 28, 2025
@github-actions github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Jun 1, 2025
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2025

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.

Copy link
Copy Markdown
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

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?

@spranav1205
Copy link
Copy Markdown
Author

Yes, I agree. Astropy support should be reserved for broader application anyway...

@github-actions github-actions Bot added the closed-by-bot Closed by stale bot label Jul 2, 2025
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jul 2, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Close? Tell stale bot that this issue/PR is stale closed-by-bot Closed by stale bot visualization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide a way to enable both quantity_support and time_support in one go

4 participants