DM-13944: add id to VisitInfo #595
Conversation
There was a problem hiding this comment.
Looks good! No objections to what you have, but SkyWcs, Psf, and the ints returned by getX0 and getY0 are all immutable as well and are totally safe as properties on any object.
In fact, anything returned by shared_ptr here is also safe as a property, even if it's mutable, because modifying the object returned by the property modifies the one in the Exposure, rather than silently doing nothing. So go wild!
...with two really unfortunate exceptions: bbox and xy0, which are both mutable objects returned by value (hence as copies that will quietly not do anything if mutated). I'm kind of inclined to make an exception for them, because I think consistency across Exposure accessors is probably more important than guarding against e.g. exposure.bbox.shift(...) being a silent no-op - but the dev guide uses Exposure.bbox as an example of what not to do, so it's probably still best to obey it until/unless we RFC more liberal property-usage guidelines.
Makes me really wish we had FrozenExtent2I and FrozenBox2I down in geom, though.
|
Thanks @TallJimbo : I've added the rest of what I think are safe. I agree that we should RFC bbox and xy0, if we want to add properties for them; bbox also has the origin kwarg to the getter, which maybe means we should maybe keep it as only a getter? Do you have suggestions for tests I should write to double check that modification of shared_ptr members gets passed down for each of these? Since the getters and properties call the same C++, I'm guessing that aspect is already well tested elsewhere? |
| */ | ||
| explicit VisitInfo(table::RecordId exposureId, double exposureTime, double darkTime, | ||
| daf::base::DateTime const &date, double ut1, lsst::geom::Angle const &era, | ||
| lsst::geom::SpherePoint const &boresightRaDec, | ||
| lsst::geom::SpherePoint const &boresightAzAlt, double boresightAirmass, | ||
| lsst::geom::Angle const &boresightRotAngle, RotType const &rotType, | ||
| coord::Observatory const &observatory, coord::Weather const &weather, | ||
| std::string const &instrumentLabel) | ||
| std::string const &instrumentLabel, table::RecordId const &id) |
There was a problem hiding this comment.
From butler this is a python integer (64-bit I think in registry) so I'm not entirely why this is a RecordId. Does that mean something special such that you could use it as a primary key in an afw table or something?
There was a problem hiding this comment.
RecordId is a typedef to a signed 64-bit int. Don't know if it's better or worse to use the typedef here; it guarantees consistency with the ID columns in afw.table that sometimes hold these values, but it's also not a desirable dependency (even if it's not the only occurrence of this dependency).
There was a problem hiding this comment.
I'm using the same type as exposureId, since this effectively replaces that. table::RecordId is int64.
Yeah, that's probably a reason to leave it as a getter :-(. Unless we can get rid of
Not well tested, but this aspect of the implementation is totally trivial; in C++ it's probably fair to say that the behavior is totally guaranteed by the fact that it compiles, and there's no way to test the immutable things, since they can't be modified anyway. But it's also true that someone could trivially modify one of the mutable-component getters to break the behavior, and that's the kind of thing that it'd be nice to have tests to guard against. But the set of DM devs who might actually make such a change is so small and so expert that I'm not really worried about it, either. |
parejkoj
force-pushed
the
tickets/DM-13944
branch
from
18de34e
to
814db28
Compare
Sorry, I was referring to the python aspect: testing that modifying a shared_ptr object in python also modifies the object in the container. I think what I'm really getting at is whether we are (and/or should be) testing the |
This field is the VisitId or ExposureId (depending on whether the data is a snap, or combined-snaps), and named generically so that we can use VisitId for either in the future.
detector, filterLabel, photoCalib, visitInfo are all immutable, so adding properties for them is safe. fixup
parejkoj
force-pushed
the
tickets/DM-13944
branch
from
814db28
to
fe74e73
Compare
We don't have to (and maybe can't?) set a |
|
@TallJimbo : I think you're correct, and I was just confused. My apologies. |
I also added Exposure properties for a few immutable members, since it was easy to do as part of this.