-
Notifications
You must be signed in to change notification settings - Fork 22
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
RAD-138: Level 3 Mosaic Update #334
Conversation
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #334 +/- ##
=======================================
Coverage 95.32% 95.32%
=======================================
Files 4 4
Lines 171 171
=======================================
Hits 163 163
Misses 8 8 ☔ View full report in Codecov by Sentry. |
…-138_L3Metadata
…into RAD-138_L3Metadata
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 looks good. Some things to think about for the 3 pm call:
- I don't understand very well what science.common means and we should talk about what we want there.
- The wcsinfo got a lot of name changes, sorry.
- We probably want 'program' in addition to 'survey', and let's remove the enum from survey.
- Let's talk about whether we want to temporarily give this a new name, e.g., wfi_mosaic2, to avoid breaking everything immediately. Then migrate the L3 pipeline to use the new name, maybe? Or some other approach.
- Let's make individual_image_meta very generic for now (no type requirements, etc, no contents), just saying that we have it.
…-138_L3Metadata
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.
Thanks Paul. A few more minor changes (sorry!). I would "approve" but I don't actually want to merge this until we look at the romancal implications.
Let's not make this change either yet, but the other thing I suspect from my discussion with Kim is that we'll send the metadata to a different archive destination (some L3 table rather than ScienceCommon). |
archive_catalog: | ||
datatype: float | ||
destination: [ScienceCommon.ra_corn4] | ||
dec_corn4: |
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.
Is there an advantage to having each corner ra/dec a separate attribute, instead of an array?
As disadvantage - accessing and setting them all is multiple separate calls as opposed to one assignment of an array.
Software sometimes needs these in order. What is the the order of these? It may be good to have them in the same order as WCS.footprint so it's consistent with other uses.
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.
The argument here is to make them accessible via a SQL query in the archive. If SQL queries support arrays better than
I remember, an array is a good choice.
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'm happy with matching WCS.footprint. I don't actually know how to specify the ordering generically and would have left this undefined; I'm happy specifying something like "counterclockwise."
I am concerned that wfi_mosaic-1.0.0 doesn't include basic information like filename, etc. Why has that information been removed? |
Are you referring to just the information in the basic schema? In general, things not in the L3 metadata are because they are unneeded and/or inapplicable. If you have keywords that are needed for db population / MAST / etc. please recommend them. |
Yes, I was referring to the basic schema. The database does not need these fields. I am unsure if MAST does. If they are not needed I'm fine with leaving them out. |
Needed to remove regression errors in romancal
RCAL-704 Update schema for resample usage
there seems to be issues with tag definitions which is currently not understood.
create new mosaic associations meta block
add coordinates and ref_file to mosaic meta
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.
LGTM
Resolves RAD-138
Closes #331
This PR updates the contents of the WFI_Mosaic model schema to a more streamlined metadata design for level 3 products.
Checklist
CHANGES.rst
under the corresponding subsection