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-24575: Add some more exposure metadata fields #350
Conversation
14fe350
to
6a80486
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 we should add all of these to visit
as well, and add code to DefineVisitsTask
to copy/average/whatever them from the constituent exposures.
As it is, an expanded data ID for calexp
or src
can't contain any of these values, because those are all visit
-based, not exposure
-based, and that's a bit of a problem, because people are probably interested in that metadata at least as often as they're interested in exposure metadata.
name: object | ||
type: string | ||
length: 64 | ||
doc: Object of interest for this observation or survey field name. |
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 is probably not worth fighting, because I get how ubiquitous it is... but "object" is super overloaded everywhere. Is "target" taken, or does that imply something different in an important context?
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 confused because I made the change to target_name this morning.... Maybe I didn't push. I also added them to visit.
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, that's exactly what I did. Sorry. If you look on the obs_base ticket you'll see that it has target_name.
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.
Do you mean also to add ra/dec?
No description provided.