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

remove duplicate times from Tamborra, Walk models #161

Merged
merged 2 commits into from
Dec 17, 2021

Conversation

JostMigenda
Copy link
Member

Input files for the Tamborra_2014, Walk_2018 and Walk_2019 models had multiple lines for certain times; either complete duplicates or with negligible numerical differences.

This trips up some algorithms (e.g. scipy.interpolate.pchip raises ValueError: 'x' must be strictly increasing sequence.), so this PR deletes those duplicate times.

@JostMigenda JostMigenda added bug Something isn't working SupernovaModel Implementing/correcting supernova model labels Dec 13, 2021
@JostMigenda
Copy link
Member Author

Once this is merged into SNEWPY, SNEWS2/sntools#40 will add support for these models to sntools.

@Sheshuk
Copy link
Contributor

Sheshuk commented Dec 14, 2021

This looks like insufficient time precision in the simulation output script.
Would it make sense to add such cleanup when we read the models, so if we later have more models from the Garching group, we don't have to manually clean them up?

@JostMigenda
Copy link
Member Author

I think I’d prefer it if a human takes a look at any such issue in the input file, as a sanity check; to confirm whether it’s actually just numerical noise like here or whether there’s a more serious issue.

Plus, if we publish model files it’s always gonna be possible that someone wants to use them independently of SNEWPY, so eliminating the problem at the root instead of fixing it in snewpy.models would be safer.

Copy link
Contributor

@sybenzvi sybenzvi left a comment

Choose a reason for hiding this comment

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

All of these changes look fine except for models/Tamborra_2014/s27.0c_3D_dir1_LS220_nux where a large number of lines were deleted. Those do not appear to be duplicates so it's not obvious why this was done.

@JostMigenda
Copy link
Member Author

JostMigenda commented Dec 15, 2021

Ouch 😣 not sure how that happened; thanks for noticing it!
I have a meeting now but can fix that tomorrow.

@JostMigenda JostMigenda requested a review from sybenzvi December 16, 2021 19:08
@github-actions
Copy link

github-actions bot commented Dec 16, 2021

Result of Benchmark Tests

Benchmark Min Max Mean
python/snewpy/test/test_snowglobes.py::test_simulation_chain_benchmark 4.70 4.79 4.74 +- 0.04

Copy link
Contributor

@sybenzvi sybenzvi 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 restoring the deleted fluxes, this looks ready to go.

@sybenzvi sybenzvi merged commit 5723189 into main Dec 17, 2021
@sybenzvi sybenzvi deleted the JostMigenda/WalkTamborraDuplicateLines branch December 17, 2021 15:19
@JostMigenda
Copy link
Member Author

For future reference, here’s a quick script to remove duplicates:

filename = 'models/Walk_2019/s40.0c_3DBH_dir1_LS220_nue'

fin = open(filename, 'r')
fout = open(filename+'_mod', 'w')

prev_t = -1

for line in fin:
    this_t = float(line.split()[0])
    if this_t <= prev_t:
        print(f"Deleted duplicate line at t={this_t}")
    else:
        fout.write(line)
    prev_t = this_t

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working SupernovaModel Implementing/correcting supernova model
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants