DM-35671: Remove gen2 reference loaders. #286
Conversation
There was a problem hiding this comment.
Don't remove the LoadReferenceObjectsTask rst file: it contains useful information about the catalog schema. We do still have DM-11044 about finding a better permanent home for these. It's no longer a Task, so I think it would go into the generic module topic type, just renamed to match the new class name:
https://developer.lsst.io/stack/generic-guide-topic-type.html
The way loadReferenceObjects.py was refactored makes it look like you added a bunch of code, but I think it was just shuffled around in the file. Is this just git doing a wonky diff?
I'd like to move all of loadReferenceObjects.py into referenceObjectLoader.py (and similarly for the config class) to try to get the names consistent. Maybe that can't be done on this ticket, but it'd be nice to try to start moving that direction.
| @@ -777,14 +776,515 @@ class ReferenceObjectLoader(ReferenceObjectLoaderBase): | |||
| Logger object used to write out messages. If `None` a default | |||
| logger will be used. | |||
| """ | |||
| ConfigClass = LoadReferenceObjectsConfig | |||
|
|
|||
| def __init__(self, dataIds, refCats, log=None, config=None, **kwargs): | |||
There was a problem hiding this comment.
Can we get rid of the **kwargs, since they're not used?
There was a problem hiding this comment.
I'm worried that could break some existing code (if somebody was erroneously passing a butler= for example). The problem was that usage was incorrect but never warned. So what I'm going to do is check if **kwargs is non-empty and throw up a deprecation warning.
| class LoadIndexedReferenceObjectsConfig(LoadReferenceObjectsConfig): | ||
| ref_dataset_name = pexConfig.Field( | ||
| dtype=str, | ||
| default='cal_ref_cat', | ||
| doc='Name of the ingested reference dataset' | ||
| doc='Name of the ingested reference dataset', | ||
| deprecated='This field is no longer used. It will be removed after v24.', |
There was a problem hiding this comment.
How did I miss that deprecation?
There was a problem hiding this comment.
I can't answer that one!
|
Oh wow, me moving the code around seems to have really confused the github change display. Sorry about that, but I think that's beyond the scope of this ticket. |
erykoff
force-pushed
the
tickets/DM-35671
branch
from
3037f42
to
7622c57
Compare
There was a problem hiding this comment.
Two more small comments
I think you can rename tests/nopytest_ingestIndexRef... and the related ingestIndexTestBase to convert. Or we could save that for DM-31698 (if so, please note as such on that ticket). And test_referenceObjectLoader and it's class could be renamed to match?
| def __init__(self, dataIds, refCats, log=None, config=None, **kwargs): | ||
| if kwargs: | ||
| warnings.warn("Instantiating ReferenceObjectLoader with additional kwargs is deprecated " |
There was a problem hiding this comment.
Oh, please file a ticket about that removal.
tests/test_loadReferenceObjects.py
Outdated
| class ReferenceObjectLoaderBaseTestCase(lsst.utils.tests.TestCase): | ||
| """Test case for ReferenceObjectLoaderBase abstract base class. | ||
| class ReferenceObjectLoaderTestCase(lsst.utils.tests.TestCase): | ||
| """Test case for ReferenceObjectLoader. |
There was a problem hiding this comment.
This line isn't necessary: that's obvious from the name. The sentence below probably could go too, since there's no more ABC. Maybe rename or put in a sentence about how this tests the more generic parts of it, not the actual catalog loading.
| def __init__(self, dataIds, refCats, log=None, config=None, **kwargs): | ||
| if kwargs: | ||
| warnings.warn("Instantiating ReferenceObjectLoader with additional kwargs is deprecated " | ||
| "and will be removed after v25.0", FutureWarning) |
There was a problem hiding this comment.
Please use stacklevel=2 here so that the warning appears to come from the caller code -- it makes it easier to find which of their lines had the errant kwarg. Generally stacklevel is always going to be useful.
erykoff
force-pushed
the
tickets/DM-35671
branch
2 times, most recently
from
ba55491
to
438cef5
Compare
erykoff
force-pushed
the
tickets/DM-35671
branch
from
438cef5
to
614aa0d
Compare
This also consolidates code from the no-longer-necessary
ReferenceObjectLoaderBasebase class. It does not currently update naming conventions.