Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
SCSB-111: Improve documentation #483
SCSB-111: Improve documentation #483
Changes from 6 commits
43d6e9d
9f8c216
5ee8d21
ad5f658
275707f
f205bc5
c72f876
b957e24
d51a0e0
fb48cac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 thename
argument in theFrame2D
initializer?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.
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.
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.
@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)?
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 wonder if a paragraphs or two before this example outlining the general structure of a WCS object would be helpful. Any objection @nden?
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.
What if the current "Getting Started" is replaced by an earlier section:
@mcara does this address your comment?
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.
Yes! This is a great introduction.
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 like this too.
Is the frame optional? I thought it's required.
Suggest adding
linear
->consists of a linear pipeline of steps
so that it's clear it's not an arbitrary graph.
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.
Now we have a complete WCS object. In the next example, we will use it to convert pixel coordinates (1, 2) to sky...
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've modified it to something similar.
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.
shouldn't this go into the example in
fits_analog.rst
?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.
Again, do you mean duplicating (with needed changes) it there rather than moving?
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.
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.
So modified, though I really like how pixelship sounds.