Move detectors params handling completely in detectors class - #947
Conversation
thehrh
left a comment
thehrh
left a comment
There was a problem hiding this comment.
As a final remark, if update_param_values_detector should remain (still used a few times in analysis.py after all), it would benefit from a more detailed docstring than
"Modification of the update_param_values function to use with the Detectors class"
, in particular, why it is needed and when to use it (compare explanation in update_param_values).
| def test_Detectors(verbosity=Levels.WARN): | ||
| from pisa.analysis.analysis import update_param_values_detector | ||
|
|
||
| def test_Detectors(): |
There was a problem hiding this comment.
Can you come up with a test which demonstrates what you fixed (not least because the notebook was unable to prevent #946)?
| "metadata": {}, | ||
| "source": [ | ||
| "When checking the chi2 value from the fitted model, you maybe see that it is around 113, while in the minimizer loop we saw it converged to 116. It is important to keep in mind that in the fit we had extended the metric with prior penalty terms. When we add those back we get the identical number as reported in the fit." | ||
| "When checking the chi2 value from the fitted model, you maybe see that it is around 111, while in the minimizer loop we saw it converged to 114. It is important to keep in mind that in the fit we had extended the metric with prior penalty terms. When we add those back we get the identical number as reported in the fit." |
There was a problem hiding this comment.
Relating to this notebook, it really doesn't need cells such as these, which were originally taken from the public 3-yr example notebook:
The two pipelines are quite different, with most complexity in the neutrino pipeline, that has several Stages and free parameters
or
While the muon pipleine is rather simple
etc.
Basically, all the content that has nothing to do with the Detectors functionality should be removed. Instead, just include a link to the public 3-yr example notebook at the top (for details about how pipelines work).
Also, you could add more instructions and hints with respect to working with the Detectors class in this notebook (setting parameters correctly, when are changes propagated, ...). A more tutorial-like notebook would be really useful to include in the sphinx docs (https://icecube.github.io/pisa/docs/tutorials.html).
|
Are you done? I could live with leaving the |
|
How would you proceed with #932? If I understand correctly, this will now render unnecessary the workaround you described there, because the state of the attached |
|
resolves #932 |
2 participants
This PR