SNEWS2 / snewpy

A Python package for working with supernova neutrinos
https://snewpy.readthedocs.io
BSD 3-Clause "New" or "Revised" License
26 stars 19 forks source link

Bug fix in SimpleRate #176

Closed soso128 closed 2 years ago

soso128 commented 2 years ago

Hi, Sorry I inadvertently removed the Snowglobes inheritance in the SimpleRate class. This commit adds it back.

soso128 commented 2 years ago

I also fixed a bug in the Convert_to_ROOT.py script: the script worked fine for a single detector but didn't update interaction channels properly when processing multiple detectors.

JostMigenda commented 2 years ago

Hi @soso128, thanks for fixing this so quickly! Two things:

  1. Can you take a look at https://github.com/soso128/snewpy/blob/soso128_test/python/snewpy/snowglobes_interface.py#L300 – I think a few words accidentally got deleted there; it should probably say “Path to the SNOwGLoBES installation”, just like in the base class?
  2. Could we add a few tests for this, to ensure we immediately notice if this part of the code ever breaks again? (See python/snewpy/test/test_snowglobes.py and …/snowglobes_integrationtest.py for similar tests that could be adapted.)
soso128 commented 2 years ago

Hi, thank you for your feedback! I fixed the docstring and added a code to the test suite (is it automatically accounted for? I didn't see any test file list so I assumed so)

soso128 commented 2 years ago

Ok, after a few issues the tests are now incorporated and seem to run fine.

JostMigenda commented 2 years ago

Excellent, thanks for the changes!

Regarding the test suite, SNEWPY has two separate sets of tests: unit tests (which run automatically on every commit) and integration tests (which only run when manually triggered, e.g. by requesting a review as I did just now).

Both unit and integration tests automatically include all tests in files which follow the conventional test_*.py file name pattern. The simplerate_integrationtest.py which you added doesn’t match that (like the previously added snowglobes_integrationtest.py), so you need to manually add it in https://github.com/SNEWS2/snewpy/blob/main/.github/workflows/integration.yml#L58. (Note that GitHub is a bit restrictive when it comes to editing workflows; if you run into issues, please try it in the web editor.)

One final question: I notice that the number of events in the two crosscheck tables—one used with SNOwGLoBES, one with SimpleRate—is within rounding error for most detectors, but differs by 3× for HALO and over 20× for IceCube. What’s going on there? Is that simply because the difference between smeared and unsmeared event numbers is unusually large for these detectors? Or is there something more complicated going on?

soso128 commented 2 years ago

Hi Jost,

Thanks for your reply, I edited the workflow (which got associated with its own pull request it seems) and added the import for test_snowglobes.py. I was also very surprised to find such large discrepancies between the smeared and unsmeared rates for some experiments. I verified that, using my scripts, the smeared rates corresponded to the ones quoted in the test code, which is why I ended up trusting the unsmeared rates. But yes, I am still very surprised by such a large discrepancy. For IceCube it might be due to the fact that the energy smearing is actually huge.

soso128 commented 2 years ago

Sorry for having given you more work and thanks for fixing the remaining issues!

JostMigenda commented 2 years ago

Don’t worry, it happens. (Maybe that’s a good reminder to run tests locally, too—the GitHub Actions are an important safety net, but running tests locally makes it much quicker to discover and fix these tiny bugs.)