-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
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.
|
👋 Thank you for your draft pull request! Do you know that you can use |
examples/io/plot_fits-image.py
Outdated
There was a problem hiding this comment.
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)
examples/io/split-jpeg-to-fits.py
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this 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
@@ -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? | |||
--------------------------------------------------------- | |||
|
There was a problem hiding this comment.
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).
docs/io/fits/appendix/faq.rst
Outdated
|
||
How do I access data stored as a table in a multi-extension FITS file? | ||
---------------------------------------------------------------------- | ||
|
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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. 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I finally found it: https://docs.astropy.org/en/stable/io/unified.html#fits
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? | ||
------------------------------------------------- |
There was a problem hiding this comment.
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.
docs/io/fits/appendix/faq.rst
Outdated
.. _sphx_glr_generated_examples_io_modify-fits-header.py: | ||
|
||
How do I edit a FITS header? | ||
---------------------------- |
There was a problem hiding this comment.
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.
Is your plan to push commit to my PR? If so, we should coordinate. Thanks for the quick review! |
Co-authored-by: Simon Conseil <contact@saimon.org>
cb67fa2
to
bfbe8d7
Compare
@saimn , I accepted your one suggestion (and also applied it to the other entry with similar It sounds like you want to handle the rest of your comments as direct push to this PR? If so, feel free now. Thanks! |
@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. |
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! |
@saimn , I cannot approve my own PR, so feel free to approve when you are done. Thanks! |
@pllim - I think I'm done. Approving so you can merge if you're happy with the changes :) |
There was a problem hiding this 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. 😉
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a 2D nupy array that we can visualize with matplotlib: | |
a 2D numpy array that we can visualize with matplotlib: |
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") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done!
Thanks for the improvements, @saimn ! |
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.