DM-15857 Use correct aliases for old schemas #397
Conversation
parejkoj
force-pushed
the
tickets/DM-15857
branch
2 times, most recently
from
eabb6de
to
27f03b5
Compare
TallJimbo
force-pushed
the
tickets/DM-15857
branch
from
8a89854
to
f299121
Compare
| @@ -99,6 +102,7 @@ FitsSchemaInputMapper::FitsSchemaInputMapper(daf::base::PropertyList &metadata, | |||
| _impl->version = lsst::afw::table::Schema::VERSION; | |||
| } | |||
| _impl->version = metadata.get("AFW_TABLE_VERSION", _impl->version); | |||
| _impl->type = metadata.get("AFW_TYPE", _impl->type); | |||
There was a problem hiding this comment.
Although it's no longer used directly, we might as well keep it around, I think?
| // transform units to lowercase. | ||
| std::string units(item.tunit); | ||
| std::transform(units.begin(), units.end(), units.begin(), [](char c) { return std::tolower(c); } ); | ||
| return includes(units, "count") || includes(units, "dn") || includes (units, "adu"); |
There was a problem hiding this comment.
Oh joy: we use all of these units? Have we standardized this yet, or do we need an RFC for it?
There was a problem hiding this comment.
We certainly don't anymore. I'm pretty sure that at one point we used dn, but that may predate afw::table, and I'm not certain we ever used adu. I was just trying to be thorough.
| // Replace the last n characters of a string with a new suffix string, returning the result as a new string | ||
| std::string replaceSuffix(std::string const &s, std::size_t n, std::string const &suffix) { | ||
| return s.substr(0, s.size() - n) + suffix; | ||
| bool hasInstFluxUnits(FitsSchemaItem const & item) { |
There was a problem hiding this comment.
Maybe name this isFluxField, since it's doing more than just checking the units?
There was a problem hiding this comment.
Clever solution to fall-through the v2 and v1 aliasing, so that the Sigma->Err happens first, then flux->fluxErr after.
Did you revert the ps1-refcat-v1.fits file to AFW_TYPE=SOURCE? We should do that, so it's consistent with most of our existing refcats.
I recommend deleting the existing sourceTable-v2.fits file and using the sourceCatalog-hsc file for all the v2 tests, since it's much less "fake". We should get a similar file for the v1 tests, since some of these problems could have been avoided if we had a "real" v1 file in here for testing. On the other hand, we probably won't care a year from now, so...
Given that your fix completely ignores the AFW_TYPE field for old data, I'm not sure it's worth keeping my script for updating refcats around.
I did not (yet). Will do.
Sorry, I forgot to mention that I tried this, and ran into trouble in that
Yeah, for that reason I don't think this is worth the effort. |
|
@parejkoj do you want to put your script for converting refcats on a new branch so I can remove it from this one? |
|
You can just delete it, I don't think it's worth saving. |
TallJimbo
force-pushed
the
tickets/DM-15857
branch
from
a075769
to
9c8b8e4
Compare
This mostly leaves behavior unchanged, but we now correctly add instFlux -> flux aliases to the middle of fields when desired.
TallJimbo
force-pushed
the
tickets/DM-15857
branch
2 times, most recently
from
c8e93ae
to
fbb7a03
Compare
|
The saga continues: the last fix broke a lot of packages downstream in Jenkins (at least daf_butler and meas_astrom), because it turns out that git-committed artificial test SourceCatalogs that use fields without units in what are now InstFlux slots are pretty common. Rather than manually fix all that test data, I've added a bit more backwards compatbility to afw, in the slot mechanisms: if an |
TallJimbo
force-pushed
the
tickets/DM-15857
branch
from
fbb7a03
to
4614000
Compare
No description provided.