DM-34186: Add has_simulated_content and group_counter_* #55
Conversation
timj
changed the title
Codecov Report
@@ Coverage Diff @@
## main #55 +/- ##
==========================================
+ Coverage 77.79% 78.12% +0.33%
==========================================
Files 36 36
Lines 2968 3045 +77
Branches 471 476 +5
==========================================
+ Hits 2309 2379 +70
- Misses 542 548 +6
- Partials 117 118 +1
Continue to review full report at Codecov.
|
timj
force-pushed
the
tickets/DM-34186
branch
2 times, most recently
from
6fd5b16
to
816351a
Compare
| @@ -336,4 +336,25 @@ class PropertyDefinition: | |||
| "int", | |||
| int, | |||
| ), | |||
| "has_simulated_content": PropertyDefinition( | |||
| "Boolean indicating whether any part of this observation was" " simulated.", "bool", bool, None, None | |||
There was a problem hiding this comment.
Ha. Thank you black (was on two lines before black got involved). Will fix.
| None, | ||
| None, | ||
| ), | ||
| "group_counter_end": PropertyDefinition( |
There was a problem hiding this comment.
How do we know the observation counter for the end of the exposure group at the time the observation is taken? Surely weather or instrumental problems can ruin plans and therefore the header. Are you expecting that we will skip exposure identifiers?
There was a problem hiding this comment.
A modern visit is defined by the tuple of start_num, end_num. If the second observation does not turn up as expected the next observation will have a different start_num (or will not be there at all) so there won't be a problem. When visits are defined it will define the visit with just the first one in. I do complain in define-visits if I get the second file of a pair since that implies the first one must have been taken.
| path: str, | ||
| *, | ||
| force_dict: Literal[False], | ||
| ) -> Union[ObservationGroup, MutableMapping[str, Union[MutableMapping[str, Any], ObservationInfo]]]: |
There was a problem hiding this comment.
Might be worth aliasing the MutableMapping[str, Union[MutableMapping[str, Any], ObservationInfo]] since you use it multiple times.
MutableMapping[str, Any] too.
There was a problem hiding this comment.
What I'd really like is to fix ObservationInfo so it can take a Mapping[str, Any].
| def process_index_data( | ||
| content: MutableMapping[str, Any], *, force_metadata: bool = False, force_dict: bool = False | ||
| ) -> Union[ObservationGroup, MutableMapping[str, Union[MutableMapping[str, Any], ObservationInfo]]]: | ||
| ... |
There was a problem hiding this comment.
Aren't you missing a combination? What do I get if force_metadata = True and force_dict = True?
There was a problem hiding this comment.
force_metadata = True makes force_dict irrelevant but I'll see if it makes a difference.
There was a problem hiding this comment.
I had a quick look and it seemed to trigger a bunch of warnings from the other overloads that I'd rather not try to sort out at the moment. I was going down a maze of twisty passages all alike.
| @@ -266,7 +321,7 @@ def process_index_data( | |||
| obs_infos: List[ObservationInfo] = [] | |||
| # This type annotation is really MutableMapping[str, ObservationInfo] | |||
| # but mypy needs it to look like the function return value. | |||
There was a problem hiding this comment.
I wonder if this would be fixed if you added the missing overload?
There was a problem hiding this comment.
force_metadata = False is the only way to get to this branch so I don't think the missing overload matters.
* has_simulated_content has become necessary now that we are taking ComCam data in the lab that is pretending to be on sky. * group_counter is needed to allow the new LSSTCam visit definitions to work. Headers now say they are N of M so the first and last values for observation_counter can be determined from a header.
This is a hack to allow old index/sidecar files containing serialized ObservationInfo to be read back in with defaults filling in the recent property additions.
booleans are stored as YAML boolean so there is no processing to be done.
This tests that the fallback constructor code triggers correctly.
has_simulated_contenthas become necessary now that we are taking ComCam data in the lab that is pretending to be on sky.group_counter_*are needed to allow the new LSSTCam visit definitions to work. Headers now say they are N of M so the first and last values forobservation_counter_*can be determined from a header.