DM-13711: Add type of observation information to visitInfo #652

parejkoj · 2022-08-29T23:23:40Z

No description provided.

kfindeisen

Looks good, style comments only.

kfindeisen · 2022-09-02T00:02:51Z

include/lsst/afw/image/VisitInfo.h

@@ -89,6 +89,7 @@ class VisitInfo : public table::io::PersistableFacade<VisitInfo>, public typehan
     * @param[in] id  Identifier of this full focal plane data.
     * @param[in] focusZ Defocal distance of main-cam hexapod in mm. 0 is in focus.;
     *                   Extra-focal is negative while intra-focal is positive.
+     * @param[in] observationType Type of this observation (e.g. science, dark, flat, bias, focus).


Is this open-ended, or is there a set of allowed values?

open-ended. Butler can't enforce specific observation types.

I don't see how the Butler is relevant here.

My question was motivated by the fact that, if this is going into VisitInfo, then it may be parsed by downstream code (otherwise why have it?). Establishing to the reader whether or not there are conventions, and if so what they are, is important for avoiding bugs.

Currently butler is the main user but I really meant that the metadata translator can't know every single observation type because instruments have many different modes of operation. You can't know the full list ever because there is no global data model of allowed observing modes. "dark" and "bias" are fairly standard but DECam has modes that HSC doesn't have and writing them all down is impossible. You have to refer to the specific instrument's documentation. Each metadata translator can know the obs types for its instrument but that's the limit.

include/lsst/afw/image/VisitInfo.h

src/image/VisitInfo.cc

kfindeisen · 2022-09-02T00:32:32Z

tests/test_visitInfo.py

@@ -364,6 +377,7 @@ def _testIsEmpty(self, visitInfo):
        self.assertEqual(visitInfo.getInstrumentLabel(), "")
        self.assertEqual(visitInfo.getId(), 0)
        self.assertTrue(math.isnan(visitInfo.getFocusZ()))
+        self.assertTrue(visitInfo.getFocusZ(), "")


Should this be assertEqual(visitInfo.getObservationType(), "")?

Thanks for catching this: because of this typo, I also missed adding the empty tests for the other new properties.

include/lsst/afw/image/VisitInfo.h

kfindeisen · 2022-09-02T00:41:28Z

python/lsst/afw/image/_visitInfo.cc

@@ -73,7 +74,8 @@ void declareVisitInfo(lsst::utils::python::WrapperCollection &wrappers) {
                        "rotType"_a = RotType::UNKNOWN,
                        "observatory"_a = coord::Observatory(nanAngle, nanAngle, nan),
                        "weather"_a = coord::Weather(nan, nan, nan), "instrumentLabel"_a = "", "id"_a = 0,
-                        "focusZ"_a = nan, "observationType"_a = "");
+                        "focusZ"_a = nan, "observationType"_a = "", "scienceProgram"_a = "",
+                        "observationReason"_a = "", "object"_a = "", "hasSimulatedContent"_a = false);


The idea of a bool field having a default is a bit scary, since both true and false look like meaningful values. Maybe add a comment justifying why false should be the default (I assume for backward-compatibility)?

Yeah, I worried about that too, but the infrastructure to make this behave as a None-able field seemed excessive (I don't think we want have nullptrs in VisitInfo). I'll add a note here, and I think to the couple of places in VisitInfo.cc that also default to false. @timj said that going forward it should always be properly populated for LSST data (and we'll fix the metadata where it's not):

https://lsstc.slack.com/archives/C2JPMCF5X/p1661906553417399?thread_ts=1661881492.710089&cid=C2JPMCF5X

I'm open to suggestions though, since I'm also worried about this causing someone to be misled when using data from other sources.

Yes, ObservationInfo will always set it so if your VisitInfo is set from raw data that have been processed in gen3 it will always be set and ObservationInfo declares it to be bool -- if someone creates their ObservationInfo and requests that the simulated content flag not be set and then passes it to VisitInfo then it will have a None value. If you are worried that this might be common then I don't know how easy it is to have a True/False/Undef state in C++.

A note is all I was asking for; I definitely don't think using something like std::optional is worth it!

(Though, given the follow-up discussion, I trust there's a better argument for its correctness than "This constructor must only be called by ObservationInfo"? I assumed it was more that any code that doesn't set hasSimulatedContent works in contexts that don't need to distinguish simulated from real data... though now that I think about it that's iffy for the same reason.)

I don't know if we actually have a use case for hasSimulatedContent in the pipeline yet: I suspect it's mostly for notebook analysis of various test stand data that was made to look like it's on-mountain. I've added notes, though I wish I had something more to say than "// default false for backwards compatibility"?

Oh, and this reminds me: it currently prints 0 or 1, since those are the C++ true/false output values. I don't think we want to use std::boolalpha, since printing will happen from python and the capitalization will make them unhelpful. So, I'm left with just having them printed as 0 or 1 and it's up to the user to interpret?

I'd prefer boolalpha, it's more reader-friendly. I think it's easier to interpret "true really means True" than "1 really means True". Though I'd forgotten how hard it is to use stream flags without side effects. 😖

kfindeisen · 2022-09-02T00:44:42Z

src/image/VisitInfo.cc

+              scienceProgram(schema.addField<std::string>(
+                      "scienceProgram", "observing program (survey or proposal) identifier", "", 0)),
+              observationReason(schema.addField<std::string>(
+                      "observationReason", "reason this observation was taken, or its purpose", "", 0)),


As with observationType, a list of standard values might be a better comment.

This is free format text at the mercy of the person doing the observing or the metadata translator's interpretation of some headers.

kfindeisen · 2022-09-02T00:51:56Z

src/image/VisitInfo.cc

+          _scienceProgram(getString(metadata, "PROGRAM")),
+          _observationReason(getString(metadata, "REASON")),
+          _object(getString(metadata, "OBJECT")),
+          _hasSimulatedContent(false) {


Why isn't this reading "HAS-SIMULATED-CONTENT"?

It reads from that below: there's no getBool method defined in this file, so I have to get it like the int64 fields, inside an if block.

kfindeisen · 2022-09-02T01:00:21Z

src/image/VisitInfo.cc

-    std::vector<std::string> keyList = {"EXPID",    "EXPTIME",     "DARKTIME",     "DATE-AVG",    "TIMESYS",
-                                        "TIME-MID", "MJD-AVG-UT1", "AVG-ERA",      "BORE-RA",     "BORE-DEC",
-                                        "BORE-AZ",  "BORE-ALT",    "BORE-AIRMASS", "BORE-ROTANG", "ROTTYPE",
-                                        "OBS-LONG", "OBS-LAT",     "OBS-ELEV",     "AIRTEMP",     "AIRPRESS",
-                                        "HUMIDITY", "INSTRUMENT",  "IDNUM",        "FOCUSZ",      "OBSTYPE"};
+    std::vector<std::string> keyList = {"EXPID",
+                                        "EXPTIME",
+                                        "DARKTIME",
+                                        "DATE-AVG",
+                                        "TIMESYS",
+                                        "TIME-MID",
+                                        "MJD-AVG-UT1",
+                                        "AVG-ERA",
+                                        "BORE-RA",
+                                        "BORE-DEC",
+                                        "BORE-AZ",
+                                        "BORE-ALT",
+                                        "BORE-AIRMASS",
+                                        "BORE-ROTANG",
+                                        "ROTTYPE",
+                                        "OBS-LONG",
+                                        "OBS-LAT",
+                                        "OBS-ELEV",
+                                        "AIRTEMP",
+                                        "AIRPRESS",
+                                        "HUMIDITY",
+                                        "INSTRUMENT",
+                                        "IDNUM",
+                                        "FOCUSZ",
+                                        "OBSTYPE",
+                                        "PROGRAM",
+                                        "REASON",
+                                        "OBJECT",
+                                        "HAS-SIMULATED-CONTENT"};


Yeah, that's what clang-format did, since HAS-SIMULATED-CONTENT is so much longer than the rest.

parejkoj · 2022-09-06T23:08:43Z

I've added two explicit stringification tests: I know that's generally implementation dependent, but I discovered some bugs (missing comas and spaces) in my initial version and figured making it explicit would help with that. It also makes the "boolalpha" printing more obvious.

This shouldn't have been in this if statement.

obsType fixup

scienceProgram, observationReason, object, hasSimulatedContent are all requested by the commissioning team; adding them now means less file version number churn.

I noticed I'd made some typos in my stringification, and this should help ensure such things don't happen by making the stringification more explicit in the future.

parejkoj force-pushed the tickets/DM-13711 branch from e17c2b8 to ea50d19 Compare August 31, 2022 00:13

kfindeisen approved these changes Sep 2, 2022

View reviewed changes

parejkoj force-pushed the tickets/DM-13711 branch 2 times, most recently from c6d821e to a4f5593 Compare September 6, 2022 23:06

parejkoj force-pushed the tickets/DM-13711 branch 2 times, most recently from c11d0c3 to 00ce055 Compare September 9, 2022 18:31

parejkoj added 6 commits September 12, 2022 14:16

Cleanup VisitInfo docstring

8cac739

Move focusZ metadata setter out of id if-block

7149e67

This shouldn't have been in this if statement.

clang-format on VisitInfo.cc

02444df

Add observationType string field to visitInfo

b7ba747

obsType fixup

Add requested fields to VisitInfo

0ad4c95

scienceProgram, observationReason, object, hasSimulatedContent are all requested by the commissioning team; adding them now means less file version number churn.

Add complete test of printed output

159bf26

I noticed I'd made some typos in my stringification, and this should help ensure such things don't happen by making the stringification more explicit in the future.

parejkoj force-pushed the tickets/DM-13711 branch from 00ce055 to 159bf26 Compare September 12, 2022 21:16

parejkoj merged commit 4fea24e into main Sep 12, 2022

parejkoj deleted the tickets/DM-13711 branch September 12, 2022 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-13711: Add type of observation information to visitInfo #652

DM-13711: Add type of observation information to visitInfo #652

parejkoj commented Aug 29, 2022

kfindeisen left a comment

kfindeisen Sep 2, 2022

timj Sep 2, 2022

kfindeisen Sep 6, 2022 •

edited

timj Sep 6, 2022

kfindeisen Sep 2, 2022

parejkoj Sep 6, 2022

kfindeisen Sep 2, 2022

parejkoj Sep 6, 2022

timj Sep 6, 2022

kfindeisen Sep 6, 2022 •

edited

parejkoj Sep 6, 2022

parejkoj Sep 6, 2022

kfindeisen Sep 6, 2022 •

edited

kfindeisen Sep 2, 2022

timj Sep 2, 2022

kfindeisen Sep 2, 2022

parejkoj Sep 6, 2022 •

edited

kfindeisen Sep 2, 2022

parejkoj Sep 6, 2022

parejkoj commented Sep 6, 2022

DM-13711: Add type of observation information to visitInfo #652

DM-13711: Add type of observation information to visitInfo #652

Conversation

parejkoj commented Aug 29, 2022

kfindeisen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Sep 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Sep 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Sep 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parejkoj Sep 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parejkoj commented Sep 6, 2022

kfindeisen Sep 6, 2022 •

edited

kfindeisen Sep 6, 2022 •

edited

kfindeisen Sep 6, 2022 •

edited

parejkoj Sep 6, 2022 •

edited