DM-35655: Remove gen2 jointcal code and tests #225
Conversation
python/lsst/jointcal/jointcal.py
Outdated
| @@ -511,11 +424,11 @@ class JointcalConfig(pipeBase.PipelineTaskConfig, | |||
| default=20, | |||
| ) | |||
| astrometryRefObjLoader = pexConfig.ConfigurableField( | |||
There was a problem hiding this comment.
I think this should be a pexConfig.ConfigField with dtype=LoadIndexedReferenceObjectsConfig or dtype=LoadReferenceObjectsConfig. That should make the configurations work the same, and we aren't using the task via makeSubtask so the retargetable ConfigurableField isn't necessary.
As to which of the dtypes to use: If you set this to ReferenceObjectLoader (as the current PR) or LoadReferenceObjectsConfig (the config with that task) then any existing configs that set ref_dataset_name will break without first having a deprecation period. It may be that these configs have all been flushed from the system already, but I worry ... Oh, I just checked, and they haven't. Because this config wasn't previous deprecated (that was an oversight) it is currently persisted with the jointcal configs that are in recent runs. So I think this has to be dtype=LoadIndexedReferenceObjectsConfig until after 24 (assuming we can get that properly deprecated in the meantime).
There was a problem hiding this comment.
Yes, that all sounds like a good plan.
python/lsst/jointcal/jointcal.py
Outdated
| @@ -1235,7 +997,7 @@ def _do_load_refcat_and_fit(self, associations, defaultFilter, center, radius, | |||
| On-sky radius to load from reference catalog. | |||
| name : `str` | |||
| Name of thing being fit: "astrometry" or "photometry". | |||
| refObjLoader : `lsst.meas.algorithms.LoadReferenceObjectsTask` | |||
| refObjLoader : `lsst.meas.algorithms.ReferenceObjectLoaderBase` | |||
There was a problem hiding this comment.
I think this has to be lsst.meas.algorithms.ReferenceObjectLoader which has the actual loading code in it.
There was a problem hiding this comment.
Ah, shoot, I thought the base class defined the necessary API, but it actually doesn't. It probably should; I don't know if we plan to keep that gen2/3 compatibility base task or not (one could argue that a different gen3 refcat loader could be built using the same API).
There was a problem hiding this comment.
The base and derived class should probably be consolidated. I don't think anything would be referring to be base on its own (because as you have noticed, it doesn't actually do anything useful).
There was a problem hiding this comment.
I wrote that base class! It does something very useful: gave the gen3 loader functionality that it was missing and got rid of a lot of bugs in it! I think the problem in using it to define an API was that the gen2/gen3 loaders didn't share an API in their e.g. LoadPixelBox methods. I could see using it to define a gen3 refloader API having some utility.
There was a problem hiding this comment.
It definitely needed to exist as long as gen2 was around. My point is that the need for a split between base and derived is probably no longer necessary and the two could be spliced together.
Cleanup comments referring to gen2 or gen3. dataIds.py was only used by the gen2 test runner to identify visits to load in each tract.
This is done via Connections in gen3, and has been marked deprecated for years.
LoadIndexedReferenceObjectsTask and ref_dataset_name are gen2 only, but we have to keep using the former for the ConfigField until it has followed one deprecation cycle (`ref_dataset_name` exists in currently-used configs).
parejkoj
force-pushed
the
tickets/DM-35655
branch
from
27663b8
to
8652d17
Compare
No description provided.