Conversation
jmeyers314
force-pushed
the
tickets/DM-54146
branch
from
3cc2d8b to
17911c0
Compare
erykoff
left a comment
erykoff
left a comment
There was a problem hiding this comment.
A few questions/comments; looks good overall.
|
|
||
|
|
||
| class IntrinsicZernikes(IsrCalib): | ||
| """ |
|
|
||
| super().__init__(**kwargs) | ||
|
|
||
| if table is not None: |
There was a problem hiding this comment.
This table seems to be different from the calib table? A docstring might help explain what this is.
| newCalib = IntrinsicZernikes.fromTable(self.calib.toTable()) | ||
| self.assertEqual(newCalib, self.calib) | ||
|
|
||
| def test_yaml_roundtrip(self): |
There was a problem hiding this comment.
We don't usually save in yaml (I didn't know this would work?) we save in ecsv which should be the text-based approach (I think?).
There was a problem hiding this comment.
yaml is listed here!
But happy to add an ecsv test too. In fact, adding that let me discover that .yaml and .fits files are apparently silently overwritten by writeText and writeFits, but attempting to overwrite an ecsv is an error. Not sure if you care to symmetrize that or not (i.e., add overwrite=True here ).
|
Addressed comments. But looking at this again fresh, I'm wondering if I need to worry more about details like "field angle but in what coordinate system" and should it be field angle or focal plane position... |
2 participants
No description provided.