Sheshuk/snowglobes interface

Sheshuk commented 2 years ago

Closes #126 Closes #130

A new python interface for running SNOwGLObES implemented:

Using Jinja2 library to prepare the configuration files from a template
Using pandas to store and manipulate data tables So this adds two dependencies, but make code much smaller and simpler.

If many flux files provided, it tries to run them asynchronously. But it's not possible to parallelize running bin/supernova subprocess, because SNOwGLoBES always uses the same configuration file supernova.glb

The new interface collects the output files right after the process finished, so there is no need for cleanup, and also disk i/o should be reduced. And now there's no need for a separate simulate and collate steps.

Usage is simple:

>>> from snewpy.snowglobes import SNOwGLoBES
>>> sng = SNOwGLoBES()
>>> results = sng.run('models/Bollig_2016/fluence_Bollig_2016_s11.2c_AdiabaticMSW_NMO.dat',detector='icecube')
>>> table = results[0].weighted.smeared
>>> table.sum() #events per channel
channel
ibd                304251.462816
nue_e                4895.468609
nuebar_e             1275.958651
numu_e                803.749995
numubar_e             644.040633
nutau_e               796.368011
nutaubar_e            649.163208
nue_O16              3473.060781
nuebar_O16           3709.538704
nc_nue_O16             24.800743
nc_nuebar_O16          20.355537
nc_numu_O16            15.465031
nc_numubar_O16         24.038376
nc_nutau_O16           15.465031
nc_nutaubar_O16        24.038376
dtype: float64
>>> table.sum().sum() #total events
320622.97449880163

The previous functions snowglobes.simulate and snowglobes.collate now use this interface and are adapted for the backward compatibility. Now simulate stores the resulting tables in the hdf5 file, and collate aggregates the nc_* and _e channels like

github-actions[bot] commented 2 years ago

Result of Benchmark Tests

Benchmark	Min	Max	Mean
python/snewpy/test/test_snowglobes.py::test_simulation_chain_benchmark	5.76	6.06	5.93 +- 0.11

JostMigenda commented 2 years ago

Hi @Sheshuk! Two high level comments right up front:

Separating simulate and collate into two independent steps is not a design issue that needed to be fixed, but a feature. Detectors, smearing matrices and cross-sections don’t usually change frequently, whereas analysis code often does; so it is useful to have a “cached” version of the results on disk and just run collate at the start of an analysis script, instead of having to re-run SNOwGLoBES every single time.
I would set a high bar for any changes to the interface that are not backwards-compatible; especially such a short time after submitting the ApJ and JOSS papers. Changing the interface will lead to significant confusion for users; we should make sure we have very good reasons to do so and, following SemVer principles, we should only do it in a new v2.0.

Sheshuk commented 2 years ago

I would set a high bar for any changes to the interface that are not backwards-compatible

I agree. It's not hard to make the output backward compatible, while keeping the new functionality. Will do that.

Separating simulate and collate into two independent steps is not a design issue that needed to be fixed, but a feature. Detectors, smearing matrices and cross-sections don’t usually change frequently, whereas analysis code often does; so it is useful to have a “cached” version of the results on disk and just run collate at the start of an analysis script, instead of having to re-run SNOwGLoBES every single time.

SNOwGLoBES running time is negligible, compared to the current collate step. I agree that it's feasible to store the results of the simulation, but one needs to store collated data tables, not the individual output files of the SNOwGLoBES - and this is handled by collate step. Then the data tables can be processed.

Sheshuk commented 2 years ago

Here's a timing benchmark with simulate+collate steps for the main branch (old) and current branch (new), processing the file, generated with generate_time_series for a single detector (icecube):

~/work/snews2/snewpy(main*) _ pytest-benchmark compare 0020 0019                                                                                                            

--------------------------------------------------------------------------------------------- benchmark: 2 tests --------------------------------------------------------------------------------------------
Name (time in s)                                   Min                Max               Mean            StdDev             Median               IQR            Outliers     OPS            Rounds  Iterations
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_simulation_chain_benchmark (0019_new)     12.2361 (1.0)      12.6197 (1.0)      12.4153 (1.0)      0.1894 (1.0)      12.3697 (1.0)      0.3710 (1.24)          2;0  0.0805 (1.0)           5           1
test_simulation_chain_benchmark (0020_old)     77.6742 (6.35)     78.1286 (6.19)     77.8271 (6.27)     0.1979 (1.04)     77.7252 (6.28)     0.2999 (1.0)           1;0  0.0128 (0.16)          5           1
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Legend:
  Outliers: 1 Standard Deviation from Mean; 1.5 IQR (InterQuartile Range) from 1st Quartile and 3rd Quartile.
  OPS: Operations Per Second, computed as 1 / Mean

Sheshuk commented 2 years ago

Restored the integration test (just removed the hardcoded paths, so it can be used not only in the github.runner)
Made the returning values backward compatible. Now the integration test passes.
Simulate now stores data in hdf5 file, and collate reads it and processes.

Please have a look, any suggestions are welcome.

JostMigenda commented 2 years ago

Many thanks for these changes!

I just checked out the branch to try it out and noticed that installing with pip install . doesn’t currently work, since the PyTables module (used internally by pandas.HDFStore which writes/reads the files in simulate() and collate()) requires installing the HDF5 C library separately.

I’ve tried using np.save/np.load instead and found that it not just works fine, but is also noticeably faster:

Time to write files:
HDFStore: 0.178929
np.save: 0.000516

Time to read files:
HDFStore: 0.022347
np.load: 0.004025

I’ve made a new commit with this change.

On a separate note, I’ve seen that you use removesuffix() in the code. That’s a new feature in Python 3.9, so we’d have to require a quite new version. This seems a bit aggressive; for comparison, the policy of NumPy (and many others packets in the scientific Python ecosystem) is to support Python 3.7 until end of December and require 3.9 from April 2023.

Finally, running pytest python/snewpy/test/test_snowglobes.py I get one error:

____________________________________________________________________ ERROR at setup of test_simulation_chain_benchmark _____________________________________________________________________
file /Users/jost/Documents/Academia/SNEWS/Software/snewpy/python/snewpy/test/test_snowglobes.py, line 48
  @pytest.mark.timing
  def test_simulation_chain_benchmark(benchmark):
E       fixture 'benchmark' not found
>       available fixtures: cache, capfd, capfdbinary, caplog, capsys, capsysbinary, doctest_namespace, monkeypatch, pytestconfig, record_property, record_testsuite_property, record_xml_attribute, recwarn, sng, tmp_path, tmp_path_factory, tmpdir, tmpdir_factory
>       use 'pytest --fixtures [testpath]' for help on them.

/Users/jost/Documents/Academia/SNEWS/Software/snewpy/python/snewpy/test/test_snowglobes.py:48

I guess the issue is that I’m missing pytest-benchmark, which you’ve added to the integration.yml but not to requirements.txt. However, since both pytest and pytest-benchmark are not required for using snewpy, I’d probably only add them as extras_require in setup.py and remove pytest from requirements.txt; so it’s not automatically installed with pip install snewpy but only with e.g. pip install snewpy[dev]. What do you think?

Sheshuk commented 2 years ago

I’ve tried using np.save/np.load instead and found that it not just works fine, but is also noticeably faster

Good catch, thanks! I didn't know numpy deals with nested dictionaries of tables. I hesitated to use PyTables in the first place (although it worked for me after pip install tables, probably HDF5 was already installed).

On a separate note, I’ve seen that you use removesuffix() in the code. That’s a new feature in Python 3.9, so we’d have to require a quite new version. This seems a bit aggressive; for comparison, the policy of NumPy (and many others packets in the scientific Python ecosystem) is to support Python 3.7 until end of December and require 3.9 from April 2023.

Oh, I didn't know that. I'll get rid of it, off course (fixed in https://github.com/SNEWS2/snewpy/pull/132/commits/f87ac099a53758d2f9d46cb246c9284e9f58ad38) . I suppose we'll need to include also running snowglobes tests with [3.7, 3.8] in our CI.

I’d probably only add them as extras_require in setup.py and remove pytest from requirements.txt; so it’s not automatically installed with pip install snewpy but only with e.g. pip install snewpy[dev]. What do you think?

Yes, that's a good idea! We can also put the snewpy[doc] requirements there as an extra, instead of having a separate file in docs/.

JostMigenda commented 2 years ago

Added them as extras in setup.py, as discussed. Good point about the docs’ requirements, though that should probably be a separate PR. I’ve opened an issue so we don’t forget.

… and speaking of docs: I don’t think we need to update them, since these changes are behind the scenes and don’t affect the user interface; but let me know if I’m missing something?

evanoconnor commented 2 years ago

Trying this now, I get the following:

asyncio.run() cannot be called from a running event loop

I have 300 hundred flux files in my tar ball, if that is useful. I will debug myself later, but any insight would be helpful.

Sheshuk commented 2 years ago

@JostMigenda I've updated the docstrings, so maybe we can add it in the snowglobes section of the documentation, as "Low-level interface" part?

Sheshuk commented 2 years ago

@evanoconnor can you provide the code you're running? So I could debug

evanoconnor commented 2 years ago

ok @Sheshuk re: asyncio.run() cannot be called from a running event loop, this occurs for me with the SNOwGLoBES_usage.ipynbfile in the doc/nbdirectory. So maybe something with my python/jupyter setup...

Sheshuk commented 2 years ago

Oh, so this is because Jupyter has its own loop. I used async functions to use the time we're waiting for the SNOwGLoBES process more efficiently. Maybe I should put my calculation in a separate thread to avoid the async loops collision.

Also I'll add a separate async test case

Sheshuk commented 2 years ago

@evanoconnor could you try again now? It should be fixed by https://github.com/SNEWS2/snewpy/pull/132/commits/972c3e2f6964644646866d9be9a7f4f3a7be9b8f.

evanoconnor commented 2 years ago

Yes, this now works and I get the same results as in the main branch without the super long collate stage. Nice!

Is it easy to include a progress bar for the number of fluence files?

I guess the warnings can be suppressed with a log alert level or something?

Sheshuk commented 2 years ago

Yes, this now works and I get the same results as in the main branch without the super long collate stage. Nice!

Great! Could you please measure the time for both branches? (%%time magic command in Jupyter cell)

Is it easy to include a progress bar for the number of fluence files?

The files are being processed asynchronously, i.e. somewhat in parallel, so we only have an input and output points. I'll think of possible solutions. For example, we could run them in some smaller bunches.

I guess the warnings can be suppressed with a log alert level or something?

Looking into it. Can you paste here the warnings?

Sheshuk commented 2 years ago

Oh, now I see the warnings. They're mostly from logging.info, so it's possible to configure it to be less verbose by default.

evanoconnor commented 2 years ago

all times in seconds

this branch: fluence calculation: 43 running snowglobes (wc100kt30prct): 50.2 collating (wc100kt30prct): 13.4

main branch: fluence calculation: 43.1 running snowglobes (wc100kt30prct): 55.4 collating (wc100kt30prct): 247

for the warnings, the efficiency files not found one shows up a bunch (for scint, since that one doesn't have an efficiency, it is for every fluence file I run). This is useful, but a normal snowglobes users doesn't get such a warning, so not critical for us to relay I think, it could be a fairly low level warning, maybe off by default.

Sheshuk commented 2 years ago

What is the desired behavior? Now when we try to process a detector with undefined efficiency/smearing we write a warning, and return Exception instead of the result. IMO the user should be warned, that something went wrong while processing his input. I can reduce the number of warnings - making a check for the detector before trying each flux file.

JostMigenda commented 2 years ago

@Sheshuk I think the "Low-level interface" section in the docs is useful; thanks! Here’s the documentation for that branch on readthedocs for proofreading; it looks like there are still a few formatting issues.

Sheshuk commented 2 years ago

@JostMigenda fixed the docs, please check in case I missed something, @evanoconnor I reduced the number of warning messages, so it's only one for each detector without efficiencies. Another problem is d2O detector - it doesn't have smearing files for the channels, so it crashes. I didn't hardcode the list of detectors if you pass detector_input="all", so it reads all detectors known to SNOwGLoBES. After I fix this, I think, it's ready to merge, unless you have more suggestions.

JostMigenda commented 2 years ago

Just some minor cleanup. No more obvious issues from my end; thanks a lot for being so responsive!

Sheshuk commented 2 years ago

Ugh, this d2O experiment also lacks the xsec files 😞 I guess I'll just have to drop it from the definition of the detectors in snowglobes.simulate (detector_input="all") case.

Sheshuk commented 2 years ago

Okay, now I think it's done. It doesn't produce many warnings unless someone specifically provides incorrect input/experiment. Thanks for the feedback and corrections! Ready to merge when approved, unless there are more suggestions.

Sheshuk commented 2 years ago

And I also have the problem with detectors and materials ending with _he. I'll work on that tomorrow and let you know when I'm ready,

JostMigenda commented 2 years ago

Okay, let’s wait for that (and for Evan’s confirmation).

On the administrative side: This is a pretty large PR that effectively re-writes a whole component of the software. Given that the JOSS review is currently ongoing (and one reviewer has already completed their review), it’s probably preferable to finish that before merging this PR? (Alternatively, we could create a separate JOSSReview/v1.1 branch from the current main branch, use that to continue the JOSS review and tag v1.1 off of it when it is finished; while merging this PR to main and continuing feature development there.)

Sheshuk commented 2 years ago

Okaay, looks like the *_he experiments are defined on a different energy grid - with 400 points, not 200 as others, that's why it was crashing. Now I have it hardcoded - which is fine with the current version of SNOwGLoBES, but probably we'll want to have some flexibility with it in the future.

Sheshuk commented 2 years ago

@JostMigenda I've fixed the _he detectors, so now everything seems to be done.

Alternatively, we could create a separate JOSSReview/v1.1 branch from the current main branch, use that to continue the JOSS review and tag v1.1 off of it when it is finished; while merging this PR to main and continuing feature development there

I like that option. Polishing the release should be decoupled from merging the new features. So, either we make a separate branch for new release, as you suggested, or make additional development branch with the same protection rules - 1 approved PR. Whatever you prefer

Sheshuk commented 2 years ago

Thanks! Yes, it's possible to make an async progressbar, but needs some work. It can be a separate task.

SNEWS2 / snewpy

Sheshuk/snowglobes interface #132

Result of Benchmark Tests