Pisa weight fitter #237
Pisa weight fitter #237
Conversation
There was a problem hiding this comment.
Hi Rasmus,
I don't have a good handle on the PISA portion of the repo, so I am happy to merge this PR without much back-and-forth. I have made one suggestion though, to update an import that was changed on a recent PR, that I suggest we add before merging. I have also suggested adding just a bit of documentation to explain the intent behind the WeightFitter class, as this is not super clear to me at present, but I'll leave that up to you. :)
| @@ -74,6 +75,174 @@ def config_updater( | |||
| updater.write(configfile) | |||
|
|
|||
|
|
|||
| class WeightFitter: | |||
| def __init__( | |||
There was a problem hiding this comment.
A docstring here, or in fit_weights, would be helpful to explain which weights are being fitted, exactly.
src/graphnet/pisa/fitting.py
Outdated
| class WeightFitter: | ||
| def __init__( | ||
| self, | ||
| database_path, |
There was a problem hiding this comment.
Here and elsewhere we are assuming that databases (as opposed to some other intermediate file format) are being used to interface with PISA. Is that something we are committing to, or just something that works for the time being but might be expanded on down the line?
There was a problem hiding this comment.
This is something we can expand upon down the line, I think. Currently, GraphNeT is only interfaced with PISA via SQL. To make it work with a new format, we need to update PISA as it relies on a dataloader for each format. See https://github.com/icecube/pisa/tree/master/pisa/stages/data for reference. They are easy to write, so it shouldn't be a big thing.
import statement Co-authored-by: Andreas Søgaard <andreas.sogaard@gmail.com>
…raphnet into pisa_weight_fitter
adds the ability to use pisa to fit physical weights to each events in a database