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.

Sign up for GitHub

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

Jump to bottom

U/krughoff/dm 1296 #9

Merged
merged 4 commits into from Feb 25, 2015
Merged

U/krughoff/dm 1296 #9

merged 4 commits into from Feb 25, 2015

Conversation

SimonKrughoff
Copy link
Contributor

No description provided.

@jchiang87
Copy link
Contributor

Hi Simon,

I'm putting my code review comments here...not sure what, if anything, you want me to do at the JIRA issue.

fitsUtils.py

These comments are based on commit 30eef22

  • lines 7, 14:
    • Apply convention for in-place object modification with that object as first argument (like a method).
    • Are these intended for use outside of module? if not, prepend "_" to function name (It seems these are intended for use outside of the module, since these tested are in unit tests, in which case these functions should have docstrings.)
  • line 25: fix typos
  • line 53: Add Exception.message to warning output.
  • lines 117-122: variables are named "dmt_" whereas the header keywords are "DTM_" (the comment also uses "DMT"). Make these consistent.
  • lines 125-126: Some comment on why these are the right values for FLIPX and FLIPY would be helpful here (I'm not sure that they are).
  • lines 143-147: 'BIASSEC' is read from the FITS file hdu. How are keyword arrays specified in FITS headers? Is an array-like BIASSEC an actual use case that will be encountered in practice?
  • line 223: fix typos (incomplete comment?).

Ok to merge after fixing typos and adding comments where requested. Other changes are up to you if you want to do them.

@SimonKrughoff
Copy link
Contributor Author

@jchiang87 thanks for doing this. I fixed everything you asked for. In response to the question about array BIASSEC values, this is part of the NOAO mosaic definition (as I understand it). I think we will pick a saner default when we go back through and choose new defaults.

Simon Krughoff added 4 commits February 24, 2015 17:24
@SimonKrughoff
Add classes for mapping between FITS and python
daf02c4 
We need to map FITS keywords to python attributes.  These classes
will help in mapping between the two by providing defaults and transform
classes.
@SimonKrughoff
Add the body of the detector construction class
146f3cb 
This class constructs a Detector from the header cards
in a FITS file.
@SimonKrughoff
Add a few convenience methods
15c72b5 
We, of course, need to build a detector.  buildDetector does that.
The method makeCalib provides a hook for putting in calibration information:
exposure time, flux calibration, etc.  The makeExposure method
puts it all together to provide an Exposure that can be used to
assemble amps, for example.
@SimonKrughoff
Add unit test for cameraGeom.fitsUtils
739841c
@SimonKrughoff SimonKrughoff merged commit 739841c into master Feb 25, 2015
@ktlim ktlim deleted the u/krughoff/DM-1296 branch August 25, 2018 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@SimonKrughoff @jchiang87