SCSB-111: Improve documentation #483

perrygreenfield · 2023-11-14T19:12:29Z

Closes issues:

docs mention saving to AsdfInFits which is not available in asdf 3.0 #465 docs mention saving to AsdfInFits which is deprecated and will be removed in asdf 3.0
Documentation refers to deprecated asdf.fits_embed #453 Documentation refers to deprecated asdf.fits_embed
Example on front page hard to follow #363 Example on front page hard to follow
Documentation/Example for IFU does not work #350 Documentation/Example for IFU does not work
documentation clarification #281 documentation clarification (regarding inverses)

codecov · 2023-11-14T19:15:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b483673) 87.28% compared to head (fb48cac) 87.28%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #483   +/-   ##
=======================================
  Coverage   87.28%   87.28%           
=======================================
  Files          22       22           
  Lines        3821     3821           
=======================================
  Hits         3335     3335           
  Misses        486      486

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

CHANGES.rst

docs/gwcs/fits_analog.rst

mcara · 2023-11-16T16:10:16Z

docs/gwcs/fits_analog.rst

+  >>> det2sky = shift_by_crpix | rotation | tan | celestial_rotation
+  >>> det2sky.name = "linear_transform"
+
+Create a ``detector`` coordinate frame and a ``celestial`` ICRS frame.


Why do we need this detector frame? Can't we just bunch all transforms together from detector all the way to the sky? Also, what is a "detector frame"? is it just the value of the name argument in the Frame2D initializer?

Generally the frames are information containers - they hold the units on the axes, the names of the axes, the axes type and physical type. You could create a WCS without an input frame but some functionality will not be available. For example, if the transforms are defined with units, you will need to make sure the inputs are with units, so any time you pass input in the case above, the inputs need to have u.pix attached to them. This can be annoying.
In addition the input frame does not always represent a detector and imo it's good to have the information on what inputs are expected.

@nden comment should be added to the docs. My comment was intended to prompt a discussion/explanation about organization of the (gwcs) pipelines. For example, why do some pipelines have multiple steps (JWST WCS comes to mind)?

I wonder if a paragraphs or two before this example outlining the general structure of a WCS object would be helpful. Any objection @nden?

What if the current "Getting Started" is replaced by an earlier section:

Basic Structure of a GWCS Object -------------------------------- The key concept to be aware of is that a GWCS Object consists of a pipeline of steps; each step contains a transform (i.e., an Astropy model) that converts the input coordinates of the step to the output coordinates of the step. Furthermore, each step has an optional coordinate frame associated with the step. The coordinate frame represents the input coordinate frame, not the output coordinates. Most typically, the first step coordinate frame is the detector pixel coordinates (the default). Since no step has a coordinate frame for the output coordinates, it is necessary to append a step with no transform to the end of the pipeline to represent the output coordinate frame. For imaging, this frame typically references one of the Astropy standard Sky Coordinate Frames of Reference. The GWCS frames also serve to hold the units on the axes, the names of the axes and the physical type of the axis (e.g., wavelength). Since it is often useful to obtain coordinates in an intermediate frame of reference, GWCS allows the pipeline to consist of more than one transform. For example, for spectrographs, it is useful to have access to coordinates in the slit plane, and in such a case, the first step would transform from the detector to the slit plane, and the second step from the slit plane to sky coordinates and a wavelength. Constructed this way, it is possible to extract from the GWCS the needed transforms between identified frames of reference. The GWCS object can be saved to the ASDF format using the `asdf <https://asdf.readthedocs.io/en/latest/>`__ package and validated using `ASDF Standard <https://asdf-standard.readthedocs.io/en/latest/>`__ There are two ways to save the GWCS object to a files: - `Save a WCS object as a pure ASDF file`_

@mcara does this address your comment?

Yes! This is a great introduction.

I like this too.

Furthermore, each step has an optional coordinate frame associated with the step.

Is the frame optional? I thought it's required.

a GWCS Object consists of a pipeline of steps

Suggest adding linear -> consists of a linear pipeline of steps
so that it's clear it's not an arbitrary graph.

docs/gwcs/fits_analog.rst

docs/gwcs/ifu.rst

mcara · 2023-11-16T16:45:13Z

docs/index.rst


-Create a transform which rotates the inputs using the ``PC matrix``.
+The more general case where the detector is not aligned with north, would have
+a rotation transform after the pixelship and pixelscale transformations to


Suggested change

a rotation transform after the pixelship and pixelscale transformations to

a rotation transform after the pixelshift and pixelscale transformations to

So modified, though I really like how pixelship sounds.

docs/index.rst

mcara · 2023-11-16T16:49:57Z

docs/index.rst

+- Adjusting pixel coordinates such that the center of the array has 
+  (0, 0) value (typical of most WCS definitions, but any pixel may
+  be the reference that is tied to the sky reference, even the (0, 0)
+  pixel, or even pixels outside of the detector).
+- Scaling pixels such that the center pixel of the array has the expected
+  angular scale. (I.e., applying the plate scale)
+- Projecting the resultant coordinates onto the sky using the tangent
+  projection. If the field of view is small, the inaccuracies resulting
+  leaving this out will be small; however, this is generally applied.
+- Transforming the center pixel to the appropriate celestial coordinate 
+  with the approprate orientation on the sky. For simplicity's sake,
+  we assume the detector array is already oriented with north up, and
+  that the array has the appropriate parity as the sky coordinates.
+
+
+The detector has a 1000 pixel by 1000 pixel array.
+
+For simplicity, no units will be used, but instead will be implicit.


shouldn't this go into the example in fits_analog.rst?

Again, do you mean duplicating (with needed changes) it there rather than moving?

docs/gwcs/fits_analog.rst

docs/index.rst

CHANGES.rst

docs/gwcs/fits_analog.rst

nden · 2023-11-30T19:39:51Z

docs/gwcs/fits_analog.rst

+  >>> det2sky = shift_by_crpix | rotation | tan | celestial_rotation
+  >>> det2sky.name = "linear_transform"
+
+Create a ``detector`` coordinate frame and a ``celestial`` ICRS frame.


I like this too.

Furthermore, each step has an optional coordinate frame associated with the step.

Is the frame optional? I thought it's required.

a GWCS Object consists of a pipeline of steps

Suggest adding linear -> consists of a linear pipeline of steps
so that it's clear it's not an arbitrary graph.

perrygreenfield added 4 commits November 9, 2023 10:45

fix mask issue for IFU example

43d6e9d

remove asdf in fits examples

9f8c216

remove asdf in fits examples

5ee8d21

final changes

ad5f658

perrygreenfield requested a review from a team as a code owner November 14, 2023 19:12

perrygreenfield requested review from nden and mcara November 14, 2023 19:12

perrygreenfield added 2 commits November 14, 2023 14:18

update CHANGE.rst with PR number

275707f

add missing doctest-skip directives

f205bc5

mcara reviewed Nov 16, 2023

View reviewed changes

nden added the documentation label Nov 21, 2023

nden reviewed Nov 21, 2023

View reviewed changes

docs/gwcs/fits_analog.rst Outdated Show resolved Hide resolved

nden reviewed Nov 21, 2023

View reviewed changes

docs/index.rst Outdated Show resolved Hide resolved

address review comments

c72f876

perrygreenfield force-pushed the improvedocs branch from 227cf7f to c72f876 Compare November 22, 2023 20:42

Merge branch 'master' into improvedocs

b957e24

nden reviewed Nov 30, 2023

View reviewed changes

nden added 2 commits December 14, 2023 06:53

Merge branch 'master' into improvedocs

d51a0e0

move change log; add text

fb48cac

nden approved these changes Dec 18, 2023

View reviewed changes

nden merged commit fd14e08 into spacetelescope:master Dec 18, 2023
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCSB-111: Improve documentation #483

SCSB-111: Improve documentation #483

perrygreenfield commented Nov 14, 2023

codecov bot commented Nov 14, 2023 •

edited

Loading

mcara Nov 16, 2023

nden Nov 21, 2023

mcara Nov 29, 2023

perrygreenfield Nov 29, 2023

perrygreenfield Nov 29, 2023

mcara Nov 29, 2023

nden Nov 30, 2023

mcara Nov 16, 2023

perrygreenfield Nov 22, 2023

mcara Nov 16, 2023

perrygreenfield Nov 22, 2023

nden Nov 30, 2023

	a rotation transform after the pixelship and pixelscale transformations to
	a rotation transform after the pixelshift and pixelscale transformations to

SCSB-111: Improve documentation #483

SCSB-111: Improve documentation #483

Conversation

perrygreenfield commented Nov 14, 2023

codecov bot commented Nov 14, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 14, 2023 •

edited

Loading