DM-35817: Turn off compatibility mode as the default. #229
Conversation
isullivan
force-pushed
the
tickets/DM-35817
branch
from
83474f3
to
5d25743
Compare
There was a problem hiding this comment.
A few suggestions, since I'd just been working on this for DM-971.
Please reword the commit message to say why you defaulted mode="convolveTemplate": it's not obvious at first glance why that's related to here (I'm assuming because that's our transition plan).
| if self.forceCompatibility: | ||
| self.mode = "convolveTemplate" | ||
| if self.forceCompatibility and not (self.mode == "convolveTemplate"): | ||
| raise ValueError("If forceCompatibility=True, then mode='convolveTemplate' must also be set") |
There was a problem hiding this comment.
Here's my suggestion for the internals fo the if; FieldValidationError gives some useful extra context.
raise lsst.pex.config.FieldValidationError(AlardLuptonSubtractConfig.forceCompatibility,
self, msg)
| @@ -252,6 +255,7 @@ def _run_and_check_images(statsCtrl, statsCtrlDetect, scienceNoiseLevel, templat | |||
| config = subtractImages.AlardLuptonSubtractTask.ConfigClass() | |||
| config.doSubtractBackground = False | |||
| config.forceCompatibility = False | |||
There was a problem hiding this comment.
I would remove all the config.forceCompatibility = False lines from all the tests, since it now defaults to that.
tests/test_subtractTask.py
Outdated
| @@ -155,6 +155,9 @@ def test_config_validate_forceCompatibility(self): | |||
| config = subtractImages.AlardLuptonSubtractTask.ConfigClass() | |||
| config.mode = "auto" | |||
| config.forceCompatibility = True | |||
| with self.assertRaises(ValueError): | |||
| config.validate() | |||
| config.mode = 'convolveTemplate' | |||
There was a problem hiding this comment.
Here's what I'd done (after the forceCompatibility=True line), and removing the two lines below:
# Ensure we're not trying to change config values inside validate().
config.freeze()
with self.assertRaisesRegex(FieldValidationError,
"forceCompatibility=True requires mode='convolveTemplate'"):
config.validate()
config = subtractImages.AlardLuptonSubtractTask.ConfigClass()
config.mode = "convolveTemplate"
config.forceCompatibility = True
# Ensure we're not trying to change config values inside validate().
config.freeze()
# Should not raise:
config.validate()
5d25743
to
2f31e82
Compare
| if self.forceCompatibility: | ||
| self.mode = "convolveTemplate" | ||
| if self.forceCompatibility and not (self.mode == "convolveTemplate"): | ||
| msg = "forceCompatibility=True requires mode='convolveTemplate'" |
There was a problem hiding this comment.
Oh, I forgot to include one thing in my suggestion: but mode was '{self.mode}'" at the end of this message (and make it an f-string).
Also set the default convolution mode to `convolveTemplate`. This is the next step in our transition to the refactored image differencing, which preserves the default functionality of always convolving the template. With `forceCompatibility=False` we can use the new code to fix a few bugs that were present in the old ImageDifferenceTask. Update subtractImages config validation.
isullivan
force-pushed
the
tickets/DM-35817
branch
from
2f31e82
to
d63f0c7
Compare
Also set the default convolution mode to
convolveTemplate