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-21215: Add FocalPlaneProjector for fgcm camera rotations. #75

Merged
merged 1 commit into from Nov 18, 2021

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Oct 20, 2021

No description provided.

Copy link

@jmeyers314 jmeyers314 left a comment

Choose a reason for hiding this comment

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

LGTM. A few small questions/comments.


wcsDict = {}

for i, detector in enumerate(self.camera):

Choose a reason for hiding this comment

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

Looks like this could just be for detector in self.camera

delta_ra_cent, delta_dec_cent, delta_ra, delta_dec for
each detector id.
"""
if not np.isfinite(orientation):

Choose a reason for hiding this comment

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

Are you intentionally using nan or inf as a sentinel for something? Should this raise an Error instead perhaps? Maybe this behavior just needs some documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some early engineering HSC data that we have tested on in the past has nans here ... but the current test data doesn't. So I'll add a warning (but not raise) if it encounters this.


Returns
-------
projectionMapping : `np.darray`

Choose a reason for hiding this comment

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

typo

# Put the reference boresight at the equator to avoid cos(dec) problems.
self.boresight = geom.SpherePoint(180.0*geom.degrees, 0.0*geom.degrees)
self.flipX = False
self.defaultOrientation = int(defaultOrientation)

Choose a reason for hiding this comment

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

Do you want to deal with -90 = 270 here or elsewhere?

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 question. I'll look and see where the best place to wrap would be.

Parameters
----------
orientation : `int`
Camera orientation in degrees. Integerized for better caching.

Choose a reason for hiding this comment

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

Can orientation be defined? Is this the angle between camera +y and North, for instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erykoff erykoff merged commit 6a90155 into master Nov 18, 2021
@erykoff erykoff deleted the tickets/DM-21215 branch November 18, 2021 19:17
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