Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a dictionary of keyword arguments for SN models + added code for ROOT output #171

Merged
merged 11 commits into from
Jan 12, 2022

Conversation

soso128
Copy link
Contributor

@soso128 soso128 commented Jan 12, 2022

This pull request contains two commits:

1- Added an argument to generate_time_series and generate_fluence in snowglobes.py: this dictionary allows to give keyword arguments to define SN models (EOS, etc...)

2- Added a new code to get ROOT 2D histograms for the rates, instead of collated text files

@JostMigenda JostMigenda linked an issue Jan 12, 2022 that may be closed by this pull request
@JostMigenda JostMigenda self-assigned this Jan 12, 2022
@JostMigenda
Copy link
Member

Thanks @soso128!

I see you’ve already added the docstrings—excellent! Unfortunately, it looks like you accidentally included some unrelated changes to collate() in 3bc87a4. Once you remove those, I think part 1 looks good.

Regarding part 2: ROOT is a very heavy dependency (in terms of time and disk space to install) and, in my experience, quite fragile when it comes to binary compatibility with different versions of Python. So I would be very hesitant before depending on it for a core feature of SNEWPY. Since this code also appears to duplicate a lot of the code from snewpy.snowglobes.collate(), maybe it would be better to have a sample script in snewpy/python/scripts/ that demonstrates how to take the tables returned by snewpy.snowglobes.collate() and write those to a ROOT file?

@soso128
Copy link
Contributor Author

soso128 commented Jan 12, 2022

Ok, done. Indeed it would be better to write a code in scripts/ for the ROOT output. I'll do this.

@soso128
Copy link
Contributor Author

soso128 commented Jan 12, 2022

I just added a script for the ROOT output (and removed the previous code)

Copy link
Member

@JostMigenda JostMigenda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for moving this to a script! It looks good, I just have a few minor suggestions to make it a little clearer to the user.

python/snewpy/scripts/Convert_to_ROOT.py Outdated Show resolved Hide resolved
python/snewpy/scripts/Convert_to_ROOT.py Outdated Show resolved Hide resolved
python/snewpy/scripts/Convert_to_ROOT.py Outdated Show resolved Hide resolved
Copy link
Member

@JostMigenda JostMigenda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good now; many thanks @soso128 for being so responsive! And congratulations on your first contribution to SNEWPY! 🎉

@JostMigenda JostMigenda merged commit f9e7556 into SNEWS2:main Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set "eos" argument for Garching models
2 participants