Skip to content

DOC: Move FITS example gallery to FAQ - #16194

Merged
pllim merged 8 commits into
astropy:mainfrom
pllim:exorcise-fits-example-gallery
Mar 18, 2024
Merged

pllim merged 8 commits into
astropy:mainfrom
pllim:exorcise-fits-example-gallery

Conversation

@pllim

@pllim pllim commented Mar 13, 2024 •

Copy link
Copy Markdown
Member

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.

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

@github-actions

Copy link
Copy Markdown
Contributor

👋 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?

@pllim pllim Mar 13, 2024 •

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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

@saimn saimn left a comment

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.

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.

Comment thread docs/io/fits/appendix/faq.rst Resolved
Comment thread docs/io/fits/appendix/faq.rst Outdated

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

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

Comment thread docs/io/fits/appendix/faq.rst Outdated

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

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.

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
Copy Markdown
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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Comment thread docs/io/fits/appendix/faq.rst Outdated
.. _sphx_glr_generated_examples_io_plot_fits-image.py:

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

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

Comment thread docs/io/fits/appendix/faq.rst Outdated
.. _sphx_glr_generated_examples_io_modify-fits-header.py:

How do I edit a FITS header?
----------------------------

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.

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

@pllim

pllim commented Mar 13, 2024

Copy link
Copy Markdown
Member Author

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
pllim force-pushed the exorcise-fits-example-gallery branch from cb67fa2 to bfbe8d7 Compare March 14, 2024 16:47
@pllim

pllim commented Mar 14, 2024

Copy link
Copy Markdown
Member Author

@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

saimn commented Mar 15, 2024

Copy link
Copy Markdown
Contributor

@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

pllim commented Mar 15, 2024

Copy link
Copy Markdown
Member Author

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

pllim commented Mar 18, 2024

Copy link
Copy Markdown
Member Author

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

@saimn

saimn commented Mar 18, 2024

Copy link
Copy Markdown
Contributor

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

@pllim pllim left a comment

Copy link
Copy Markdown
Member Author

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

Comment thread docs/io/fits/usage/image.rst Outdated
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
Copy Markdown
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:

Comment thread docs/io/fits/usage/image.rst Outdated
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@pllim
pllim deleted the exorcise-fits-example-gallery branch March 18, 2024 21:07
@pllim

pllim commented Mar 18, 2024

Copy link
Copy Markdown
Member Author

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.

2 participants