-
Notifications
You must be signed in to change notification settings - Fork 35
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
fix astropy bug #652
base: develop
Are you sure you want to change the base?
fix astropy bug #652
Conversation
I looked at the ARIANNA hard drive to try and figure out which version of astropy the file was made with originally (from our root files -> nur file output converter); it looks like it was made with an extremely old version of astropy (2.0.14), with Python 2.7. The old root files also work with astropy 5.1 as-is. |
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.
Haven't looked at this in much detail yet, but possibly this is the same issue as #481, which should have been fixed (or just prevented from occurring in the future?) by #520. If I recall correctly, the issue was that astropy.time.Time objects were serialized directly, but opening an 'old' astropy.time.Time object with a newer version of astropy.time.Time led to compatibility issues after astropy 5.0. Probably @fschlueter recalls the details better than I do, and may be able to weigh in as to why this issue has appeared again (or still).
@@ -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') |
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 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
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.
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.
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.
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 ?
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.
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.
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.
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?
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.
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
-
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. ↩
@sjoerd-bouma you remember correctly: With #520 we prevent problems for the future by not seralizing Not setting the format should not be an issue. But should does not mean that it does not I guess |
@cg-laser what is the error msg, is it the time format or the way of setting it? Maybe you can share a file with which this issue appears? |
I know this was a question for Christian but I can answer this, since I brought up the bug/error msg to him. An example file that causes this issue is this file: "Spice_750mDown_Dec30_2018_idl_10dB.nur" And this was the error message:
|
So I tried to understand a bit better whats going on. I think at some point they replaced the variable
However, I have not extensively tested it myself. I tested it on the file mentioned above and a newer file which works just with setting the format |
I took into account your comments. @aj-nielsen can you check that you can read the ARIANNA files and that you get the correct event times? |
@aj-nielsen does that work for you? |
Yes, this works for me & I can read one of the files, sorry for the delayed response. |
I think that can be merged |
What happened with this PR |
setting the time format to 'isot' results in an error when older nur files are read with a newer version of astropy (I have 5.2.2). I don't know with which version the original nur file was created.
The expected behavior is that order nur files can always be read in. This is high priority as e.g. ARIANNA data is stored as nur files.
Before merging, I will remove the warning message, but I thought I would keep it in for now in case someone wants to investigate the underlying problem. It seems that not setting the format explicitly works just fine.