search

icecube / pisa

Monte Carlo-based data analysis
http://icecube.github.io/pisa/
Apache License 2.0
19 stars 47 forks source link

Improve support for custom stages and fit methods #703

Closed atrettin closed 2 years ago

atrettin commented 2 years ago

This PR greatly improves the support for externally defined custom PISA stages and custom fit methods.

Concerning fit methods, it turns out that I had designed the BasicAnalysis class in a dumb way, I could have done it far easier with the getattr method instead of writing my own dictionary. It is now possible to easily implement custom fit methods in a sub-class, all they have to do is to follow the _fit_{method} naming scheme. This is also tested in a unit test.

The only change for analyzers is that fit_ranges has been renamed to ranges and fit_octants has been renamed to octants for consistency.

For externally defined stages, PISA will now search for stage definitions in the {stage_name}.{service_name} module if it fails to find a stage in pisa.stages.{stage_name}.{service_name}. This makes it possible to define custom stages for an analysis without changing PISA, as long as one adds the directory with the stage definitions to the PYTHONPATH.

atrettin commented 2 years ago

Also note that a lot of changes are just my editor removing trailing whitespace. When you hide whitespace in the diff view, it looks much less intimidating. :)

philippeller commented 2 years ago

Hi, this look fine to me, but let's give a chance to the high stats folks to comment

atrettin commented 2 years ago

Maybe I should add backwards compatibility to re-map fit_octants to octants and fit_ranges to ranges and issue a deprecation warning. That way, this PR should not introduce any changes to already existing analyses.

philippeller commented 2 years ago

I think that would be nice to have, and then we should also be able to merge this right away without causing other issues downstream

atrettin commented 2 years ago

Okay so now at least in theory this PR should not break anyone's configuration. It would be nice if @kayla-leonard and @ts4051 could confirm that their analysis still runs with these changes!

atrettin commented 2 years ago

Since there are no further comments, I would say that this is fine and merge later today. The unit test does check that the backwards compatibility works. The Retro-Sample people seem to be using their own fork anyway (although I hope that this PR in particular can reduce the need for forking, because analysis-specific stages can be defined externally).