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

Log Bins for snowglobes.py #206

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Log Bins for snowglobes.py #206

wants to merge 6 commits into from

Conversation

stlgolfer
Copy link

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
Copy link
Member

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.

python/snewpy/snowglobes.py Outdated Show resolved Hide resolved
#needed_offset = tedges[0] + 0.0001*u.s
#tedges = tedges + tedges[0] + 0.0001*u.s #shift so it's very close to 0
#tmin = 0.0001*u.s
#tmax+=tmin
log_edges = np.asarray([])

if tmax < 0 or tmin < 0:
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t these be <=0?

python/snewpy/snowglobes.py Outdated Show resolved Hide resolved
@JostMigenda
Copy link
Member

JostMigenda commented Sep 20, 2022

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?

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.

None yet

2 participants