Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-34644: Clean up decorrelation code #214

Merged
merged 8 commits into from May 5, 2022
Merged

DM-34644: Clean up decorrelation code #214

merged 8 commits into from May 5, 2022

Conversation

isullivan
Copy link
Contributor

No description provided.

@@ -737,7 +738,7 @@ def run(self, subExposure, expandedSubExposure, fullBBox,
subExp1 = templateExposure.Factory(templateExposure, expandedSubExposure.getBBox())

# Prevent too much log INFO verbosity from DecorrelateALKernelTask.run
logLevel = self.log.getLevel()
logLevel = self.log.level
self.log.setLevel(self.log.WARNING)
Copy link
Member

@timj timj May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really need to add the logging context manager to the utils package so that the log level resets automatically when the task below completes. I'm not overly happy with how we handle this since it really is saying that this task is too verbose by default and it's impossible to ever see the info messages. As written, if someone sets the log level to DEBUG in pipetask they won't get any debug messages out of this task and that seems wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree that code that forces the log level like this is problematic. This is code that is not used outside of the unit tests, for now at least, and I just wanted to change this line to fix a warning while building the package. This particular task will need to be refactored if we want to use it in production.

@@ -449,7 +449,7 @@ def testNoiseDiffimCorrection(self):

self._setUpSourcelessImages(svar=svar, tvar=tvar)
diffExp, mKernel, expected_var = self._makeAndTestUncorrectedDiffim()
corrected_diffExp = self._runDecorrelationTask(diffExp, mKernel)
corrected_diffExp = self._runDecorrelationTask(diffExp.clone(), mKernel)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my edification, why the change to .clone() here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other places too, actually....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's so I can modify the difference exposure in place in the decorrelation task, and not effect the tests.

@isullivan isullivan merged commit 367af9f into main May 5, 2022
@isullivan isullivan deleted the tickets/DM-34644 branch May 5, 2022 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants