-
Notifications
You must be signed in to change notification settings - Fork 12
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
Added logging to photometry functions (and two other tweaks) #150
Conversation
…misfires' with some extra logging to console I didn't intend and some logging (before the misfires) that don't happen, almost as if logging has to take a few seconds to activate.
…t logging handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, I like PRs that reduce lines of code!
Only one important thing to change here, I think, modifying the if statement I commented on below.
I'm not sure about removing all the handlers but tbh don't remember logging well enough to say anything sensible.
I'll open an issue for the a couple of logging questions/issues.
stellarphot/photometry/photometry.py
Outdated
# Set up logging (retrieve a logger but purge any existing handlers) | ||
root_logger = logging.getLogger() | ||
for handler in root_logger.root.handlers[:]: | ||
root_logger.root.removeHandler(handler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this was because of the root handler in ccdproc
? It seems like removing other handlers might mess with other people's stuff?
I don't have any better ideas, and am not sure what, if anything, to change here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got the impression from the documentation that the root handler isn't really supposed to be used if you are working on logging for a specific function/class or package... just by the fact that the docs promote to automatically set up loggers for specific modules saying
A good convention to use when naming loggers is to use a module-level logger, in each module which uses logging, named as follows:
logger = logging.getLogger(__name__)
That said, I agree, it wasn't necessarily a friendly thing to slam off any root loggers running, since someone might want to import our code into their code... and using the root
logger is not necessarily an officially discouraged behavior. But I don't think we can 'disable' the root handler and then re-activate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for fun, I checked what loggers were running with logging.Logger.manager.loggerDict.items()
... holy crap there are a lot of them! I also explored the idea of using logging.root.disabled = True
, but that apparently disabled all the logging. In fact, any attempt to affect just the root logging (e.g. setting the level on its handler) propagates to ALL the other handlers. What a bas-ackwards system of logging (I mean, I am sure there is meant to be a logic to this, but dang)....
`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something weird is going on ccdproc uses the thing you suggested when it does logging:
logger = logging.getLogger(__name__)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe one of the packages ccdproc imports, although a brief review of that code doesn't reveal any packages I don't use regularly. I suspect when you create a logger for a module, since it is a child of the root logger, a root logger must be created... maybe for some reason you are creating the root handler? I did check and I didn't see any of the mistakes I saw others point out (e.g. using the BasicConfig setup).
BTW, did you want me to turn off console logging if a log file is provided or do you want me to log to both? I rather think console feedback is necessary. |
I think there needs to be a way to disable console logging. That might mean adding way to simply disable it rather than disabling only if a log file is provided. |
Could do this as a separate PR if you just want to get this one merged... |
…ing) but make the default for it to go to console.
I just pushed a commit to add an option for console login in addition to file logging. It is True by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, thanks!
I was able to add an option to log messages in
single_image_photometry
andmulti_image_photometry
to a file. In fact, it just uses logging for all the relevant messages and returns the log to the console UNLESS the user has asked for a log file instead.Turns out my major hassle was
ImageFileCollection
also uses logging and leaves a spare root logging handler active. It was spewing extra copies of logging messages to screen. :-/Question: Logging is currently to console OR to a file. Do we want to log to console always? I don't like not offering any feedback. With the FITSFixedWarnings suppressed now (see below), the number of console messages isn't really overwhelming.
The two other tweaks:
FITSFixedWarning
warnings that occur every time a FITS file is loaded, they are ignored by the photometry routines since we know their cause is essentially harmless.reject_unmatched
parameter (that defaults to True) for themulti_image_photometry
that allows you to control whether or not to require a source be on all the images to be included in the final returned dataset. It occurred to me someone could be processing images where their source is close to their observing limit. In those situations they may want to keep all the observations, even the non-detections.EDIT: Closes #148