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-26725: Update the PTC dataset to be a proper IsrCalib #55
Conversation
Shorten reduced chi sq variable name
Change names in plotting routines Delete setattr in ptc dataset Fix bugs in plotting task
Fix bugs in creation of FITS table
Uncomment lines to fix flake8 error
a9d0e7a
to
7d08160
Compare
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 think it's just typos and me possibly being confused about things.
@@ -416,15 +419,15 @@ def evalCovModel(self, mu=None): | |||
|
|||
def getA(self): | |||
"""'a' matrix from Astier+19(e.g., Eq. 20)""" | |||
return self.params['a'].full.reshape(self.r, self.r) | |||
return np.array(self.params['a'].full.reshape(self.r, self.r)) |
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.
Doesn't reshape return an np.array? Do you need to convert `self.params['a'].full' to a numpy array?
python/lsst/cp/pipe/ptc.py
Outdated
now = datetime.datetime.utcnow() | ||
calibDate = now.strftime("%Y-%m-%d") | ||
butler = dataRef.getButler() | ||
datasetPtc.updateMetadata(setDate=True, detectorName=detName, detectorId=detector.getId(), |
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.
updateMetadata
now supports passing the camera and detector directly, which simplifies this a bit.
python/lsst/cp/pipe/ptc.py
Outdated
# Bad amp | ||
# Entries need to have proper dimensions so read/write with astropy.Table works. | ||
matrixSide = self.config.maximumRangeCovariancesAstier | ||
nanMatrix = np.empty((matrixSide, matrixSide)) |
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.
np.full((matrixSide, matrixSide), np.nan)
might be clearer.
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.
If the round-trip versions are equivalent, then I think this is done (although please consider moving that test to ip_isr).
tests/test_ptc.py
Outdated
localDataset.finalModelVars[ampName] = np.repeat(100.0, nSignalPoints) | ||
localDataset.finalMeans[ampName] = self.flux*np.arange(nSignalPoints) | ||
|
||
# Round trip (write-read) with FITS |
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.
This should test equivalence of the local dataset and the one read back from disk. tempfile
will also handle the file IO so you don't need to.
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, you want to write it to fits and yaml and then read both back in and ensure they agree with what you had originally (and yes this test should be with the calibration class code in ip_isr.
tests/test_ptc.py
Outdated
@@ -88,6 +90,62 @@ def setUp(self): | |||
self.dataset.rawExpTimes[ampName] = timeVec | |||
self.dataset.rawMeans[ampName] = muVec | |||
|
|||
def test_ptcDataset(self): |
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.
Can this be moved to ip_isr
? This tests the integrity of the calibration definition, not the measurement code.
Fix bugs related to plotting with dataset class Fix small bug
02397f1
to
cda26c8
Compare
No description provided.