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-6999: Logging framework migration #11

Merged
merged 3 commits into from Sep 2, 2016
Merged

DM-6999: Logging framework migration #11

merged 3 commits into from Sep 2, 2016

Conversation

hsinfang
Copy link

@hsinfang hsinfang commented Aug 4, 2016

Change back to use pexLog.Debug directly to facilitate logging framework migration

@isullivan
Copy link
Contributor

The code changes all look fine. Could you add to the commit message the ticket number that will remove pexLog permanently?

With the logging framework changes in pipe_base, Tasks' logs
is transitioned from pex.logging to lsst.log, and negative logging
levels are no longer used. To facilitate logging framework migration,
usage of pexLog.Debug is separated. Practically this commit reverts
part of the changes in 33520d7
@@ -154,6 +155,7 @@ def determinePsf(self, exposure, psfCandidateList, metadata=None, flagKey=None):
normalizeResiduals = lsstDebug.Info(__name__).normalizeResiduals
# Normalise residuals by object amplitude

debugLog = pexLog.Debug("meas.algorithms.psfDeterminer")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you add this debug log? This task already has a log attribute and the existing code used it.

Hsin-Fang Chiang added 2 commits August 31, 2016 18:07
The old trace/debug level of 1 was used as a warning.
Replace it by logging it to the WARN level.
@hsinfang
Copy link
Author

@r-owen Originally I thought this was a special case so I made it use pex.logging.Debug directly before we clean all pex.logging.Debug/Trace in a later ticket. At a closer look, one debug message is actually a warning message. Was pex.logging debug level of 1 used as warnings at one point long time ago? (Or probably just an bug.) In a new commit, I raised the warning message to the WARN level. And then corrected other logging calls to use log. May you please take another look and see if you agree with these changes?

@r-owen
Copy link
Contributor

r-owen commented Aug 31, 2016

I'll have a look. I also posted a table of old vs new log levels to DM-6999 as a comment

@@ -186,8 +186,7 @@ def determinePsf(self, exposure, psfCandidateList, metadata=None, flagKey=None):
sizes[i] = rmsSize

if self.config.kernelSize >= 15:
debugLog.debug(1, \
"WARNING: NOT scaling kernelSize by stellar quadrupole moment, but using absolute value")
self.log.warn("NOT scaling kernelSize by stellar quadrupole moment, but using absolute value")
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a sensible change to me

@r-owen
Copy link
Contributor

r-owen commented Aug 31, 2016

I'm not an expert on the code in question, but your changes look reasonable to me. I suspect many more such decisions will be required due to lack of trace levels in the log package.

@hsinfang hsinfang merged commit da599c6 into master Sep 2, 2016
@ktlim ktlim deleted the tickets/DM-6999 branch August 25, 2018 06:15
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