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

DM-9111 Add rotator information to HSC VisitInfo #190

Merged
merged 1 commit into from Mar 20, 2017
Merged

Conversation

jmeyers314
Copy link
Contributor

No description provided.

Copy link
Contributor

@r-owen r-owen left a comment

Choose a reason for hiding this comment

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

Looks great, though I asked for one comment to be clarified.

Please also fix the code that computes rotation angle in obs_lsstSim and obs_test, since you have changed the sign convention. (LSST needs careful checking; obs_test is likely already correct for the old standard)

* Rotation angle is position angle of focal plane +Y with respect to the coordinate system
* specified by rotType. For example, if rotType is SKY, then at a rotation angle of 0,
* North is along focal plane +Y and East is along +X or -X (flipped to match handedness if
* necessary). At rotation angle of 90, +Y will be aligned with E.
Copy link
Contributor

Choose a reason for hiding this comment

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

The above is nice and clean, but wrong for MOUNT. Please try again. I suppose you could back off to something like "The meaning of the angle is defined by rotType, which see.", but if you can find a compact way to say more without misleading for MOUNT that would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. How about:

    /**
    * Get rotation angle at boresight at middle of exposure
    *
    * The meaning of rotation angle depends on rotType.  If rotType is SKY (HORIZON), then rotation
    * angle is position angle of focal plane +Y with respect to North (+Alt).  If rotType is MOUNT,
    * then rotation angle indicates the position sent to the rotator which depends on the specific
    * rotator.  Finally, if rotType is UNKNOWN, then the meaning of rotation angle is undefined.
    *
    * As an example, if rotType is SKY, rotation angle 0 means North is along focal plane +Y and
    * East is along +X or -X (flipped to match handedness if necessary).  At rotation angle of 90,
    * +Y is along East.
    */

Copy link
Contributor

Choose a reason for hiding this comment

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

I worry about the duplication of information with rotType. That can lead to the information going out of date in one place or the other. My first inclination would be to rely on a Doxygen link to the rotation type information and put very little information here. A compromise that might suffice is to provide the most common example (other than UNKNOWN, which is all too common, alas, but self-explanatory) but not complete information, for instance:

* The meaning...depends on @ref RotType "rotType". For example, if `rotType` is SKY,
* the angle is the position angle of the focal plane +Y with respect to North.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I've switched the doc to your suggestion.

@jmeyers314 jmeyers314 force-pushed the tickets/DM-9111 branch 2 times, most recently from 8ceb0f0 to c004f26 Compare March 16, 2017 02:02
Previous documentation was ambiguous as to which direction corresponded
to increasing rotation angle.
@jmeyers314 jmeyers314 merged commit bc289e3 into master Mar 20, 2017
@ktlim ktlim deleted the tickets/DM-9111 branch August 25, 2018 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants