Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update changelog for #1113 and loader documentation #1152

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

dhomeier
Copy link
Contributor

@dhomeier dhomeier commented Jul 12, 2024

Following up on #1113 with some clarifications of its changelog that I missed to bring up to date with the last commits.
With the new options I am wondering if they should not rather go into New Features.

This is also attempting to document the reader/writer usage a bit better in the Spectrum1D loaders docs.
No fix to the general problem that there is no narrative or API documentation for the loaders themselves – the docs so far are focussing a lot on creating custom loaders, which from experience, almost no one is using (since most people needing a specific loader are contributing it to default_loaders right away), and which is not in a good shape itself IMO.
So the changes here (essentially just the first paragraphs, the rest is just formatting fixes) are trying to point the users more to the builtin help functions.

Another quirk I noticed is the read/write docs everywhere referring to CCDData. I suppose because they are directly inherited from NDIOMixin, and CCDData is probably the oldest subclass actually implementing those I/O methods, but I don't see an easy way to fix this.

@dhomeier dhomeier requested review from kelle and removed request for eteq July 12, 2024 14:27
@dhomeier dhomeier force-pushed the tabular-fits-docs branch from d6d20b3 to 8688830 Compare July 12, 2024 14:33
@dhomeier dhomeier closed this Jul 12, 2024
@dhomeier dhomeier reopened this Jul 12, 2024
@dhomeier dhomeier force-pushed the tabular-fits-docs branch from 8688830 to 9e4e5ba Compare July 12, 2024 14:44
@dhomeier dhomeier force-pushed the tabular-fits-docs branch from 9e4e5ba to 704de20 Compare July 12, 2024 15:28
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.

Document how to write out the header.

Comment on lines +113 to +117
Note that the above example, calling ``write()`` without specifying
any format, will default to the ``wcs1d-fits`` loader if the `~specutils.Spectrum1D`
has a compatible WCS, and to ``tabular-fits`` otherwise, or if writing
to another than the primary HDU (``hdu=0``) has been selected.
For better control of the file type, the ``format`` parameter should be explicitly passed.
Copy link
Member

Choose a reason for hiding this comment

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

does header need to be specified? or does it default to using what's in meta?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@kelle
Copy link
Member

kelle commented Jul 16, 2024

Just gonna throw some notes in here as we work through some examples.

  • overwrite argument doesn't seem to be documented for write.

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 for updating this, let's get it in so I can do a release soon. We can always make further clarifications/expand the docs more later if desired.

@rosteen rosteen merged commit f77c8df into astropy:main Jul 26, 2024
1 check passed
@dhomeier
Copy link
Contributor Author

Just gonna throw some notes in here as we work through some examples.

  • overwrite argument doesn't seem to be documented for write.

This was merged before I pushed my responses to this comment, will submit them in another PR.

@rosteen
Copy link
Contributor

rosteen commented Jul 26, 2024

This was merged before I pushed my responses to this comment, will submit them in another PR.

I may have misunderstood your comments last time we spoke offline, I thought this was your plan. Apologies if it wasn't!

@dhomeier dhomeier deleted the tabular-fits-docs branch July 30, 2024 12:34
@dhomeier
Copy link
Contributor Author

I may not have stated a clear plan; I think I mentioned some further updates to docstrings, but not being sure how much of the narrative docs I'd get to work on. Does not matter if they go into this PR or another follow-up though, no problems!

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

Successfully merging this pull request may close these issues.

3 participants