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

basic code example and links to Spectrum1D.write #1017

Merged
merged 2 commits into from
Feb 23, 2023

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Feb 21, 2023

This PR adds a very brief example of how to use Spectrum1D.write and links to the API docs within the main spectrum1d landing page on the docs.

See rendered docs from this PR

@kecnry kecnry marked this pull request as ready for review February 21, 2023 20:28
@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Merging #1017 (c36ca36) into main (d4b3d2f) will not change coverage.
The diff coverage is n/a.

❗ Current head c36ca36 differs from pull request most recent head bded1d7. Consider uploading reports for the commit bded1d7 to get more accurate results

@@           Coverage Diff           @@
##             main    #1017   +/-   ##
=======================================
  Coverage   70.03%   70.03%           
=======================================
  Files          64       64           
  Lines        4339     4339           
=======================================
  Hits         3039     3039           
  Misses       1300     1300           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

-----------------

Similarly, a `~specutils.Spectrum1D` object can be saved to any file format supported by
:class:`~astropy.nddata.CCDData` via the :meth:`specutils.Spectrum1D.write` method.
Copy link
Member

Choose a reason for hiding this comment

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

Why is CCDData involved here?

class Spectrum1D(OneDSpectrumMixin, NDCube, NDIOMixin, NDArithmeticMixin):

Copy link
Member Author

Choose a reason for hiding this comment

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

It comes from NDIOMixin whose API docs state CCDData 🤷

Copy link
Member

@pllim pllim Feb 22, 2023

Choose a reason for hiding this comment

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

I think NDIOMixin used CCDData as an example of implementation using the mixin. I don't think specutils has anything to do with CCDData.

https://github.com/astropy/astropy/blob/f6dab2f2853dce72e5f11d76f6f40500f7fc7019/astropy/nddata/mixins/ndio.py#L106

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, would you rather that I just point directly to NDIOMixin then, or nothing at all and just leave the link to the API docs which inherits from NDIOMixin API?

Copy link
Member

@pllim pllim Feb 23, 2023

Choose a reason for hiding this comment

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

Why not link to https://specutils.readthedocs.io/en/latest/spectrum1d.html#reading-from-a-file (with ref)? (The table has a "write" column.)

Copy link
Member Author

Choose a reason for hiding this comment

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

that is immediately before this sub-section, so I'll just update the text to mention "supported formats".

Comment on lines 98 to 99
>>> from specutils import Spectrum1D
>>> spec1d = Spectrum1D.read("/path/to/input.fits") # doctest: +SKIP
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this? Reading is already covered in https://specutils.readthedocs.io/en/stable/spectrum1d.html#reading-from-a-file

Suggested change
>>> from specutils import Spectrum1D
>>> spec1d = Spectrum1D.read("/path/to/input.fits") # doctest: +SKIP

Copy link
Member Author

Choose a reason for hiding this comment

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

Should the snippets be somewhat standalone or is it ok to assume at this point that the user already has a spectrum1D object loaded/created?

Copy link
Member

Choose a reason for hiding this comment

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

Depends on who you ask. 😆

You can ref back to the "reading" section if you want, but I think it is overkill to tell people how to read again.

@pllim
Copy link
Member

pllim commented Feb 21, 2023

@kecnry kecnry force-pushed the docs-spectrum1d-write branch from 15791c1 to c7e657a Compare February 23, 2023 14:33
@pllim pllim added the docs label Feb 23, 2023
docs/spectrum1d.rst Show resolved Hide resolved
docs/custom_loading.rst Outdated Show resolved Hide resolved
@kecnry kecnry force-pushed the docs-spectrum1d-write branch from c7e657a to 19c8923 Compare February 23, 2023 14:40
@kecnry kecnry force-pushed the docs-spectrum1d-write branch from 19c8923 to bded1d7 Compare February 23, 2023 14:41
Copy link
Member

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

LGTM now. Thanks!

No need to wait for CI. This just touch doc without doctest.

@pllim pllim merged commit 8356796 into astropy:main Feb 23, 2023
@kecnry kecnry deleted the docs-spectrum1d-write branch February 23, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants