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

Add documentation for kwargs passed to reader+writer backends #1157

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

dhomeier
Copy link
Contributor

@dhomeier dhomeier commented Jul 31, 2024

Additional follow up to #1152 in response to #1152 (comment) – @kelle, those are generally backend options and thus depend on what interface the various loaders are using; I have completed the docstrings for the two FITS loaders to specifically point to that. Again, since these are not linked to the processed docs, I put a few examples into the narrative docs (might be preferable to link to the utility functions docs, but parsing_utils again are not included even in the API docs – would be useful to have them also for the custom loader section).

I noticed that we are linking to the astropy-dev docs rather than stable, is this by design?

@dhomeier dhomeier requested a review from kelle July 31, 2024 13:43
@dhomeier dhomeier force-pushed the fits-loaders-docs branch 2 times, most recently from 32bd129 to 0ab2aad Compare July 31, 2024 14:15
@kelle
Copy link
Member

kelle commented Jul 31, 2024

I think we should add an entire section dedicated to headers with the best practices and caveats. The information you provided in a response to me is a great start. I think the section should also include an example of header round-tripping with tabular-fits. E.g, an example of adding a header to a Spectrum1D object, writing it out, and reading it back in.

Per default it is using meta['header'] both in tabular-fits and wcs1d-fits (in principle it depends on the loader, but those 2 are the only ones that provide a writer at the moment).
With update_header=True it treats the entire meta as a dictionary to update the default header from (i.e. pulls in all items that are serialisable and do not conflict with FITS keyword rules).

Copy link
Member

@kelle kelle left a comment

Choose a reason for hiding this comment

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

I think we should add a section dedicated to headers.

@rosteen
Copy link
Contributor

rosteen commented Jul 31, 2024

I noticed that we are linking to the astropy-dev docs rather than stable, is this by design?

That surprises me, certainly not by design on my part. I actually am not even sure where that's specified.

@dhomeier
Copy link
Contributor Author

The first intersphinx links, for astropy and gwcs, were using latest, those added later stable

specutils/docs/conf.py

Lines 60 to 63 in f77c8df

intersphinx_mapping['astropy'] = ('https://docs.astropy.org/en/latest/', None)
intersphinx_mapping['gwcs'] = ('https://gwcs.readthedocs.io/en/latest/', None)
intersphinx_mapping['reproject'] = ('https://reproject.readthedocs.io/en/stable/', None)
intersphinx_mapping['mpl_animators'] = ('https://docs.sunpy.org/projects/mpl-animators/en/stable/', None)

so might be accidental.

@dhomeier dhomeier force-pushed the fits-loaders-docs branch from 0ab2aad to 5e168ba Compare August 2, 2024 12:00
@dhomeier dhomeier force-pushed the fits-loaders-docs branch from 5e168ba to 5d057de Compare August 2, 2024 12:12
@dhomeier
Copy link
Contributor Author

dhomeier commented Aug 2, 2024

I think we should add an entire section dedicated to headers with the best practices and caveats.

Yes, I think that would be a good follow up PR (i.e. for a future release) for someone with a good grasp of what are now considered best practices, maybe @simontorres?
For this docs fixup I have added a small paragraph with the basics so it is at least documented that a meta exists. ;-)

Copy link
Member

@kelle kelle left a comment

Choose a reason for hiding this comment

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

Minor suggestions.

Comment on lines 135 to 138
Conversely the two provided FITS writers are saving the contents of any
``Spectrum1D.meta['header']`` (which should be an :class:`astropy.io.fits.Header`
or any object like a `dict` that can instantiate one) as the header of the
:class:`~astropy.io.fits.hdu.PrimaryHDU`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Conversely the two provided FITS writers are saving the contents of any
``Spectrum1D.meta['header']`` (which should be an :class:`astropy.io.fits.Header`
or any object like a `dict` that can instantiate one) as the header of the
:class:`~astropy.io.fits.hdu.PrimaryHDU`.
The two provided FITS writers (`fits-tabular` and `wcs-fits`) save the contents of
``Spectrum1D.meta['header']`` (which should be an :class:`astropy.io.fits.Header`
or any object like a `dict` that can instantiate one) as the header of the
:class:`~astropy.io.fits.hdu.PrimaryHDU`.

Copy link
Member

Choose a reason for hiding this comment

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

Please also add a link to the docstrings of the tabular_fits and the wcs_fits writers. I cant seem to find those currently included in the specutils docs. Maybe that's also a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the suggestions; there are no API docs to any of the loaders one could link to, and I am afraid this will be non-trivial to amend since they are dynamically loaded through the unified IO framework.
I suppose one could build upon _list_of_loaders to then recursively call Spectrum1D.read.help(format) from its list entries to generate a doc page of all the readers (and writers)...
As mentioned above the parsing_utils functions are missing as well, but that should be more straightforward to fix; just don't know where to add the right hook for Sphinx. But those are more of interest for the Custom Loader documentation. Definitely more than enough stuff for another PR, or two.

Copy link

codecov bot commented Aug 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.87%. Comparing base (d16d91a) to head (52dd41f).
Report is 37 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1157      +/-   ##
==========================================
+ Coverage   86.83%   86.87%   +0.03%     
==========================================
  Files          63       63              
  Lines        4528     4541      +13     
==========================================
+ Hits         3932     3945      +13     
  Misses        596      596              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

Thanks again, LGTM.

@rosteen rosteen merged commit c006678 into astropy:main Aug 7, 2024
11 of 13 checks passed
@dhomeier dhomeier deleted the fits-loaders-docs branch November 27, 2024 13:41
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.

3 participants