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

DOC: Move FITS example gallery to FAQ #16194

Merged
merged 8 commits into from
Mar 18, 2024

Conversation

pllim
Copy link
Member

@pllim pllim commented Mar 13, 2024

Description

This pull request is to stop using example gallery for io.fits as discussed in #16175 (comment) .

Rendered HTML: https://astropy--16194.org.readthedocs.build/en/16194/

Partially addressed the following issues:

Pro: Faster doc build. Less reliance on sphinx-gallery. One step closer to getting rid of defunct Examples Gallery.

Con: The code snippet won't be indirectly tested during example gallery building. I did not unleash doctest on them because they heavily rely on remote data and one even creates "out of memory" file.

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

Copy link

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

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

Copy link
Member Author

@pllim pllim Mar 13, 2024

Choose a reason for hiding this comment

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

Note: I removed the astropy matplotlib style in the FAQ version of this example following discussion in #16053 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

This example isn't link from anywhere else in the doc, so I removed it altogether. I really don't see any need of example to turn RGB to FITS. I have only see real life usage of the other way around. Not sure why this was added.

@pllim pllim marked this pull request as ready for review March 13, 2024 17:34
@pllim pllim requested a review from saimn as a code owner March 13, 2024 17:34
Copy link
Contributor

@saimn saimn left a comment

Choose a reason for hiding this comment

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

Thanks @pllim for moving forward on this :), I tried some time ago and forgot about it.
Some of the examples should be merged (and simplified) in the corresponding doc page, I can try to do that.

docs/io/fits/appendix/faq.rst Outdated Show resolved Hide resolved
@@ -269,11 +374,144 @@ this FAQ might provide an example of how to do this.
.. _PyTables: http://www.pytables.org/


.. _sphx_glr_generated_examples_io_create-mef.py:

How do I create a multi-extension FITS file from scratch?
---------------------------------------------------------

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this example/section, there is one already on the index page that provides the same information (could be improved a bit, at least mentioned the MEF acronym, I have an old commit for that that I can push here: saimn@705ccb4).


How do I access data stored as a table in a multi-extension FITS file?
----------------------------------------------------------------------

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we already have an example for Table.read but if not this should go somewhere else. I will try to find where later ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to find one but nothing "beginner friendly" popped up. 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

.. _sphx_glr_generated_examples_io_plot_fits-image.py:

How do I read and plot an image from a FITS file?
-------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to say this one is useless but seems we don't have an example with imshow ! Might be useful to add at the beginning of the "image" page.

.. _sphx_glr_generated_examples_io_modify-fits-header.py:

How do I edit a FITS header?
----------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, missing info should go to the "headers" page.

@pllim
Copy link
Member Author

pllim commented Mar 13, 2024

Some of the examples should be merged (and simplified) in the corresponding doc page, I can try to do that.

Is your plan to push commit to my PR? If so, we should coordinate.

Thanks for the quick review!

pllim and others added 2 commits March 14, 2024 12:37
@pllim pllim force-pushed the exorcise-fits-example-gallery branch from cb67fa2 to bfbe8d7 Compare March 14, 2024 16:47
@pllim
Copy link
Member Author

pllim commented Mar 14, 2024

@saimn , I accepted your one suggestion (and also applied it to the other entry with similar os.remove snippet).

It sounds like you want to handle the rest of your comments as direct push to this PR? If so, feel free now. Thanks!

@saimn
Copy link
Contributor

saimn commented Mar 15, 2024

@pllim - Ok thanks. I can PR to your fork if you prefer. For now I have one commit for the MEF section and I need to find a better place for the other items.

@pllim
Copy link
Member Author

pllim commented Mar 15, 2024

I can PR to your fork if you prefer

Naw. Just push to my branch here. It is simpler that way. I have no plans to add anymore commits if we have the understanding that you are taking over. Thanks!

@pllim
Copy link
Member Author

pllim commented Mar 18, 2024

@saimn , I cannot approve my own PR, so feel free to approve when you are done. Thanks!

@saimn
Copy link
Contributor

saimn commented Mar 18, 2024

@pllim - I think I'm done. Approving so you can merge if you're happy with the changes :)

Copy link
Member Author

@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, @saimn ! Just a comment and a question. 😉

Generally the image information is located in the Primary HDU, also known
as extension 0. Here, we use :func:`astropy.io.fits.getdata` to read the image
data from this first extension using the keyword argument ``ext=0`` and obtain
a 2D nupy array that we can visualize with matplotlib:
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
a 2D nupy array that we can visualize with matplotlib:
a 2D numpy array that we can visualize with matplotlib:

from astropy.io import fits
from astropy.utils.data import get_pkg_data_filename

image_file = get_pkg_data_filename("tutorials/FITS-images/HorseHead.fits")
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we really need remote data access just to show people how to imshow? 👀

We cannot reuse one of these? https://github.com/astropy/astropy/tree/main/astropy/io/fits/tests/data

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I hesitated about this one... I included it only because there was an example to migrate, and no alternative in io/fits/tests/data. It's nice showing an image in the docs but not really necessary, I'm fine with removing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since there were PRs to speed up doc build, maybe we shouldn't regress by adding a remote data plot that would render all the time? Let's remove if you don't mind. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, done!

@pllim pllim merged commit 82532c8 into astropy:main Mar 18, 2024
24 of 27 checks passed
@pllim pllim deleted the exorcise-fits-example-gallery branch March 18, 2024 21:07
@pllim
Copy link
Member Author

pllim commented Mar 18, 2024

Thanks for the improvements, @saimn !

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

Successfully merging this pull request may close these issues.

None yet

2 participants