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.

Sign up for GitHub

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

Jump to bottom

fix astropy bug #652

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

fix astropy bug #652

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7428f20
fix astropy bug
cg-laser Mar 8, 2024
d4769e2
fix
cg-laser Mar 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions NuRadioReco/modules/io/NuRadioRecoio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import astropy.time

import logging
from NuRadioReco.utilities.logging import setup_logger

import time
import os
Expand Down Expand Up @@ -49,7 +50,7 @@ def __init__(self, filenames, parse_header=True, parse_detector=True, fail_on_ve
filenames = [filenames]

self.__file_scanned = False
self.logger = logging.getLogger('NuRadioReco.NuRadioRecoio')
self.logger = setup_logger('NuRadioReco.NuRadioRecoio')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think only scripts are meant to call setup_logger, see https://nu-radio.github.io/NuRadioMC/NuRadioReco/pages/nur_modules.html#logging and/or double check with @MijnheerD. So I think l.52 and l.11 should be reverted

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, individual modules should use the getLogger function from the standard logging library. Using the setup_logger() might work, but I have not tested what the result is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why wouldn't we use the nice new standardized logging? I have experienced no error so far. And I think this is exactly want we want to do to have a uniform formatting of logger outputs and the status level. Or why would you disagree @MijnheerD @sjoerd-bouma ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the new standardized logging. The setup_logger() function is only supposed to format the parent logger, not the individual module loggers. Thanks to the inheritance properties of the loggers, the parent logger is the one who will actually report the logs (in the nice format).

One potential issue that I just thought of, is that the setup_logger() returns a logger which does not further propagate its messages. So I think if you keep l.53 as is, setting a general logging level in the scripts will not work for this module. So I second @sjoerd-bouma suggestion to revert the changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sry if I'm slow in understanding how it works. Doesn't it require that the script that imported NuRadioRecoIO initialized a logger with the setup_logger function before the NuRadioRecoIO module initializes its logger?
And I thought if we use the correct naming scheme, i.e., starting the logger name with "NuRadioReco." it will inherit from the correct logger?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, the inheritance is taken care of by the naming scheme. But the way the logging is currently set up, is such that the parent logger (i.e. the one with the name "NuRadioReco") is the one who actually handles the log messages. All module loggers simply pass on their messages to the parent logger. And it is the latter that the setup_logger() function is supposed to set up.

As the inheritance is something that comes from the standard logging module itself, there is no need to import anything from our own logging library to initialize the module loggers, they can be called using logging.getLogger() . When using the modules in a script, their loggers will link to the parent logger that is expected to be created at the beginning 1 of the script (using the setup_logger() function).

Footnotes

  1. From my testing the ordering is actually not important, i.e. the NuRadioReco logger can be created after modules are initialized (though this is not recommended). The modules loggers will find the parent logger as long as it is exists. ↩

self.logger.info("initializing NuRadioRecoio with file {}".format(filenames))
t = time.time()
if log_level is not None:
Expand Down Expand Up @@ -202,8 +203,11 @@ def _parse_event_header(self, evt_header):
err = f"Station time not stored as dict or astropy.time.Time: ({type(value)})"
self.logger.error(err)
raise ValueError(err)

station_time.format = 'isot'
try:
station_time.format = 'isot'
except AttributeError:
self.logger.warning(f"setting format to 'isot' resulted in error.")
pass

self.__event_headers[station_id][key].append(station_time)
else:
Expand Down
Loading