RCAL-919: Formalize the patches (sky cells) file (e.g., add a data model to support it as a reference fiel) #441
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #441 +/- ##
===========================================
- Coverage 97.56% 22.92% -74.65%
===========================================
Files 30 19 -11
Lines 2788 1863 -925
===========================================
- Hits 2720 427 -2293
- Misses 68 1436 +1368 ☔ View full report in Codecov by Sentry. |
| @@ -302,6 +302,10 @@ class ReadnoiseRefModel(_DataModel): | |||
| _node_type = stnode.ReadnoiseRef | |||
|
|
|||
|
|
|||
| class RomanSkycellsRefModel(_DataModel): | |||
There was a problem hiding this comment.
| class RomanSkycellsRefModel(_DataModel): | |
| class SkycellsRefModel(_DataModel): |
Do we need the Roman prefix?
There was a problem hiding this comment.
I mulled over that, more for the filename than for the class (which is clearly associated with Roman in its context). I worry that the reference file by itself may need a clear association with Roman in the event skycells end up being used for other telescopes (like JWST). It is a good question.
| meta["nxy_skycell"] = 5000 | ||
| meta["skycell_border_pixels"] = 100 | ||
| meta["plate_scale"] = 0.055 |
There was a problem hiding this comment.
| meta["nxy_skycell"] = 5000 | |
| meta["skycell_border_pixels"] = 100 | |
| meta["plate_scale"] = 0.055 | |
| meta["nxy_skycell"] = kwargs.get("nxy_skycell", 5000) | |
| meta["skycell_border_pixels"] = kwargs.get("skycell_border_pixels", 100) | |
| meta["plate_scale"] = kwargs.get("plate_scale", 0.055) |
Any reason to not use kwargs here like in the other mk_* functions?
There was a problem hiding this comment.
Probably not, but I'm loathe to make changes until the naming stuff and file structure is settled.
| ("dec_corn4", "<f8"), | ||
| ] | ||
| ) | ||
| proj_tab = np.array( |
There was a problem hiding this comment.
Is it possible to fill these with empty arrays with the structured dtype rather than hard-coding data here?
There was a problem hiding this comment.
I think so, though there is a minor benefit for showing realistic values. I'm happy to change it though (after this whole name business is settled.
There was a problem hiding this comment.
It looks like every other file in this submodule is prefixed with _ and the only "public" API is 2 functions in __init__.py.
If I remove this file I can still make a RomanSkycellsRefModel with:
mk.mk_datamodel(rdm.RomanSkycellsRefModel)I think mk_roman_skycells in _ref_files.py can be updated (to match the other maker utils) to accept kwargs for projection_regions, skycells etc to provide a way to make skycell reference files. That would allow removal of this file and make the creation of the skycell reference file consistent with the other reference files.
perrygreenfield
force-pushed
the
skycell
branch
from
f04ec7a to
1c1d6a7
Compare
perrygreenfield
force-pushed
the
skycell
branch
from
e461742 to
dc6fb11
Compare
for more information, see https://pre-commit.ci
|
When ready, we should ask Dario to try to install & use this branch to make sure he thinks everything fits. Should that happen now, or should we wait until tests pass? |
Resolves RCAL-919
This PR addresses the addition of a data model to support the skycells definition file as a reference file.
This file does not meet all the expectations of previous reference files, e.g., omitting optical_element as that is not currently relevant, so it was necessary to put conditional checks in the testing modules to deal with this.
A utility to create this reference file from an ASDF file containing the two expected tables is also included in this PR. I'm not sure it belongs, but it may be of use. (maker_utils/mk_skycell_ref.py)