Snowglobes interface refactoring

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

Snowglobes interface refactoring #126

Closed Sheshuk closed 2 years ago

Sheshuk commented 3 years ago

As I understand snowglobes interface python/snewpy/snowglobes.py inherits from several scripts. It works could use refactoring. It contains:

Repetitive lines, which can be substituted by a cycle
Multiple checks if verbose: print... it's better to use logging module
Opening and closing files instead of using context manager with open(filename) as f

Sheshuk commented 3 years ago

I can work on it, keeping the compatibility. To keep it independent from #123 I can start with simulate method. Please let me know if anyone is working on it already, so I don't interfere with your work

JostMigenda commented 3 years ago

I will occasionally clean up parts of that code that I have to work on it anyway (as in #123), but don’t have the spare time to do a pure refactoring; and I’m not working on anything else in that module right now. If you do have spare time, feel free to go ahead! However, please do be careful—it’s very easy during refactoring to miss an edge case and introduce a subtle bug. So if in doubt, perhaps better to err on the side of leaving code as it is.

sybenzvi commented 3 years ago

@JostMigenda, we have a branch protection rule for main but have not enabled PRs before merging. I'm glad to change that and then we have at least one layer of protection against unintended consequences in refactoring. @Sheshuk is a good programmer so this is nothing against his proposal... this is probably a reasonable step now that we've had a major public release.

sybenzvi commented 3 years ago

OK, we now require 1 approval before PRs can be merged. For the moment admins are exempted. Let's see how this goes for a while. If we find it clunky or not restrictive enough, we can discuss how to update the policy.

evanoconnor commented 3 years ago

As a note, the collate part is really slow, much slower than actually running snowglobes, this is likely (just a guess) a IO thing. I would welcome a look at the cause of that if someone was so inclined. It is the time limiting step when lots of files/times are involved.

Sheshuk commented 2 years ago

Hi all, I made a new interface for snowglobes. It should be faster now, doing simulate and collate in one go. However the interface is not yet backward compatible - it returns the pandas.DataFrames, instead of dicts with numpy arrays as it used to be.

Sheshuk commented 2 years ago

Updates:

The backward compatibility is fully restored: simulate and collate now produce the same files as before, and returning the dict with "header" and "data" fields.
The simulate+collate chain is now much faster: see the benchmark. @evanoconnor you're right, most of that time was spent on reading and writing the textfiles. And also iterating in python is really slow, so one can gain a lot of performance switching to numpy and pandas for reading and writing text. The performance now is limited by waiting for SNOwGLoBES to finish: it's impossible to run the processes in parallel, because they use the same file supernova.glb, and that's hardcoded. So if that changes in future SNOwGLoBES versions, we can get it even faster.

evanoconnor commented 2 years ago

Thanks for adding in the backwards compatibility. For the sake is not making our recent publications already out of date before they are officially published…

I have something I need to run in the coming days, I will check it in this branch too.

Sheshuk commented 2 years ago

Great, thanks! Please let me know if there are particular checks you'd like to make - I can add them to the integration tests. Now it only makes a rough cross-check with numbers from a table in SNEWSv2.0 paper