search

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

Log Bins for snowglobes.py #206

Open stlgolfer opened 1 year ago

stlgolfer commented 1 year ago

This PR is to continue the discussion in the tg3-detectorresponse slack channel regarding logarithmically spaced time bins in the generate_time_fluence feature in snowglobes.py. I've also implemented @JostMigenda 's ValueError idea for dealing with negative times. Please note that this PR is merely a prototype and it includes two files: data_handlers.py and log_bins.py that are necessary to testing but are not intended to be included in the final product.

JostMigenda commented 1 year ago

Thank you for this contribution! Log-spaced time bins are a great idea, and will be especially useful for pre-SN neutrinos once we officially support them in snewpy. The code looks fine to me; after removing the testing files and commented out code (and perhaps some bits of very minor cleanup) this should be okay to merge.

JostMigenda commented 1 year ago

I’m so sorry, but it seems I missed the forest for the trees in my earlier review:

if tmax <= 0 or tmin <= 0:
    raise ValueError("Cannot apply log to time windows that are less than or equal to 0. Consider adjusting model time window")

The problem is that the generate_time_series function here doesn’t let the user adjust the time window by passing tmin and tmax as arguments (only generate_fluence does!), so this error message is not actionable.

These inconsistent and confusing arguments are definitely something we need to fix as part of #209 (which just got moved a little higher up my priority list). In the meantime, should we put this PR on hold? Or is it worth adding tmin and tmax arguments to generate_time_series in this PR as well, even though we’ll overhaul the generate functions soon anyway?