DM-37242: Refactor SkyCorrectionTask #736
Conversation
leeskelvin
force-pushed
the
tickets/DM-37242
branch
6 times, most recently
from
0ad9a0d
to
6bf87d6
Compare
leeskelvin
force-pushed
the
tickets/DM-37242
branch
2 times, most recently
from
fa579f4
to
2fa23d9
Compare
| dtype=bool, | ||
| default=True, | ||
| doc="Do initial background model subtraction (prior to sky frame subtraction)?", | ||
| deprecated="This field is deprecated and will be removed after v26. See RFC-898 for further details.", |
There was a problem hiding this comment.
removed after v27, since v25 candidate is already tagged. Also, if this is just part of the renaming, suggest the field that takes its place.
There was a problem hiding this comment.
Here's a snippet from the Dev Guide:
Removal of the deprecated code occurs, at the earliest, immediately after the next major release following the release with the first deprecation release note; at the latest, immediately before the major release following. In other words, if deprecation is first noted in release 17.2.3, the code cannot be removed until after 18.0 is released and must be removed before 19.0 is released.
To my mind, v25 hasn't been fully released yet (it's still only a release candidate), but assuming that still counts, that means that the next major release is v26. This means that this deprecation effort should take place in the v26 to v27 time period, no?
There was a problem hiding this comment.
Right, after v26 is correct then. Apologies.
There was a problem hiding this comment.
Also, it looks like both doBgModel and doBgModel1 are deprecated. That's surely not what you intended.
There was a problem hiding this comment.
Following RFC-898, the first full focal plane background model step will be made mandatory. That means that doBgModel will be removed on the implementation ticket (DM-37429). However, as we're also changing the name of the task that it targets (bgModel) that means that I also need to temporarily set up a new bgModel1 config field for use from now until the deprecation is implemented. At the time of implementation, I'll remove both doBgModel and doBgModel1 (and make the relevant code block mandatory).
I can't think of a better way to do this, but if you can, happy to switch it up!
There was a problem hiding this comment.
Either way, I can't see a way to not raise DeprecatedWarning messages. So this is fine.
| default=True, | ||
| doc="Iteratively mask objects to find good sky?", | ||
| ) | ||
| bgModel = ConfigField( |
There was a problem hiding this comment.
If you are going to introduce bgModel1, then this should be deprecated and set optional=True.
There was a problem hiding this comment.
As discussed via Slack, there is no optional argument for a ConfigField. Instead, I have set the dtype for this ConfigField to the base Config class (to allow validate to pass successfully) and left comments informing the user that this field should not be used - please use bgModel1 instead.
leeskelvin
force-pushed
the
tickets/DM-37242
branch
3 times, most recently
from
37e25ee
to
4694f5c
Compare
| Detector level realization of the full background model. | ||
| bgModelMaskedImage : `lsst.afw.image.MaskedImage` | ||
| Background model from the bgModelCcd realization. | ||
| mosImage : `lsst.afw.image.exposure.ExposureF` |
There was a problem hiding this comment.
I'd recomment making this mosaicImage
| dtype=bool, | ||
| default=True, | ||
| doc="Do initial background model subtraction (prior to sky frame subtraction)?", | ||
| deprecated="This field is deprecated and will be removed after v26. See RFC-898 for further details.", |
There was a problem hiding this comment.
Right, after v26 is correct then. Apologies.
| dtype=bool, | ||
| default=True, | ||
| doc="Do initial background model subtraction (prior to sky frame subtraction)?", | ||
| deprecated="This field is deprecated and will be removed after v26. See RFC-898 for further details.", |
There was a problem hiding this comment.
Also, it looks like both doBgModel and doBgModel1 are deprecated. That's surely not what you intended.
leeskelvin
force-pushed
the
tickets/DM-37242
branch
from
4694f5c
to
b4e4fe6
Compare
|
Thanks Arun - Re: splitting commits, normally I'd agree, but in this case the act of refactoring is what led to any new features being changed. With that said, there really aren't that many new features here. The only significant user-facing change is the addition of a new output dataset type ({{calexpBackground_skyCorr_visit_mosaic}}) which was essentially already being calculated before, just not saved. I'd find it difficult right now to unpick what I consider to be part of a refactor and what actually constitutes new functionality in short order, so if it's okay with you, I'll leave it on one commit as-is. |
No description provided.