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

Add RNO-G data reader base on Mattak and a noise importer which utilises this data reader #519

Merged
merged 45 commits into from
Apr 28, 2023

Conversation

fschlueter
Copy link
Collaborator

The data reader is based on code from @KelliMichaels. I added a few features such as a (optional) preliminary baseline correction, voltage calibration, and on the fly run selection. It is also possible to filter for specific trigger types (or any other information contained in the eventInfo class).

The noise reader is quite simple at the moment. The main issue I have with it right now is that is caches all noise event in memory at the beginning with can become impractical quite soon.

@fschlueter
Copy link
Collaborator Author

Arg, this PR is based on #508. Once this is merged, I can make a rebase and the additional diff disappears...

@fschlueter
Copy link
Collaborator Author

This should be ready now. The main concerned I had regarding the noiseImporter are fixed

Copy link
Collaborator

@shallmann shallmann left a comment

Choose a reason for hiding this comment

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

Hi Felix,
I think this is a great replacement for the current readers. Few small comments/questions.

NuRadioReco/modules/io/rno_g/readRNOGDataMattak.py Outdated Show resolved Hide resolved
NuRadioReco/modules/io/rno_g/readRNOGDataMattak.py Outdated Show resolved Hide resolved
NuRadioReco/modules/io/rno_g/readRNOGDataMattak.py Outdated Show resolved Hide resolved
@fschlueter
Copy link
Collaborator Author

@shallmann I would like to merge #508 before that one.

Felix Schlüter and others added 25 commits April 13, 2023 16:16
… event index to get a specific event rather than only looping over all. Started to implement some function to get event header information
…or function (i.e., run()) and function to return a specific event (read_event). Some fixes
…performance when using the generator (i.e., the run() method) and in particular the uproot backend. Add some logger info to the noiseImporter
…election based on time. Change interface of noiseImporter to accept dict for reader arguments
@fschlueter
Copy link
Collaborator Author

I guess the failing of the unit tests has to do with the desy server which provides the antenna files, can someone confirm?

@sjoerd-bouma
Copy link
Collaborator

Should be fixed now, so I've restarted the tests

Copy link
Collaborator

@sjoerd-bouma sjoerd-bouma left a comment

Choose a reason for hiding this comment

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

Last couple of comments, hopefully

  • while I'm in favour of only having a single reader module for RNO-G data, mattak is currently not yet pip installable, and does not have a fully functioning uproot backend yet (as far as I understand). Is it worth keeping one or both old reader modules with a deprecation warning until this has been implemented? I find it hard to judge how much of an additional challenge the installation of mattak + ROOT would present to, e.g., a starting master's or bachelor's student. If the consensus is it's doable, I'm also happy to only keep the new one.
  • The old reader modules are still used in the eventbrowser, but I'm happy to leave the fix for that to a future PR

@fschlueter
Copy link
Collaborator Author

while I'm in favour of only having a single reader module for RNO-G data, mattak is currently not yet pip installable, and does not have a fully functioning uproot backend yet (as far as I understand). Is it worth keeping one or both old reader modules with a deprecation warning until this has been implemented? I find it hard to judge how much of an additional challenge the installation of mattak + ROOT would present to, e.g., a starting master's or bachelor's student. If the consensus is it's doable, I'm also happy to only keep the new one.

Maybe @cozzyd also wants to comment. AFAIK the uproot backend is fully functional. Yes mattak is not yet pip installed but I think it just have to be cloned and set in the Pythonpath for the uproot backend (correct @cozzyd). In that case it could be added into the install script (@sjoerd-bouma would you volunteer for that, I think you will be x10 faster than me).

@sjoerd-bouma
Copy link
Collaborator

sjoerd-bouma commented Apr 25, 2023

Maybe @cozzyd also wants to comment. AFAIK the uproot backend is fully functional. Yes mattak is not yet pip installed but I think it just have to be cloned and set in the Pythonpath for the uproot backend (correct @cozzyd). In that case it could be added into the install script (@sjoerd-bouma would you volunteer for that, I think you will be x10 faster than me).

Mattak still has to be compiled, unfortunately, and I think for the directory it chooses by default I needed to sudo it. I chatted to Steffen and I think we agree that for now (while mattak doesn't have a working setup.py) it would be nice to keep one old reader with a deprecation warning, would that be okay? Other than that I think this PR can be merged.

The uproot version does not need to be compiled, and should be pip-installable once RNO-G/mattak#18 has been merged. I've moved pandas over to be an optional dependency for now.

sjoerd-bouma
sjoerd-bouma previously approved these changes Apr 25, 2023
…ter the default arugments for the MattakReader changed
@fschlueter
Copy link
Collaborator Author

@sjoerd-bouma sorry, I guess I need a new approval

sjoerd-bouma
sjoerd-bouma previously approved these changes Apr 25, 2023
Copy link
Collaborator

@sjoerd-bouma sjoerd-bouma left a comment

Choose a reason for hiding this comment

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

Thanks Felix

@fschlueter fschlueter merged commit a6fdbba into develop Apr 28, 2023
9 checks passed
@shallmann
Copy link
Collaborator

Thanks Felix, this is awesome 🥇

@anelles anelles deleted the add-rnog-noise-importer branch February 14, 2024 07:32
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

5 participants