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 default binning in Buffy #857

Closed
gonzaponte opened this issue Feb 21, 2024 · 4 comments · Fixed by #874
Closed

Remove default binning in Buffy #857

gonzaponte opened this issue Feb 21, 2024 · 4 comments · Fixed by #874

Comments

@gonzaponte
Copy link
Collaborator

This line is extremely dangerous as it uses a default value that the user surely is not aware of and probably doesn't understand. This has caused troubles in the past. I believe it doesn't cause them anymore, but it's better to be safe than sorry.

return 25 * units.ns, 1 * units.mus

@paolafer
Copy link
Collaborator

What do you suggest we do with that? Return None, remove that function and use the exception directly in the code...?

@gonzaponte
Copy link
Collaborator Author

I believe now all nexus files contain the binning, right? If so I would raise an exception if the values are not found.

@paolafer
Copy link
Collaborator

I don't know if that's what you mean, but could we simply remove

return 25 * units.ns, 1 * units.mus?
The exception is raised inside the for-loop. I dont' understand how that function works exactly.

@paolafer
Copy link
Collaborator

paolafer commented May 27, 2024

I see that in the current version of the function, if no binnings are found in any of the files, the default values are returned. If we want to change that (sensible), we have to modify the test that deals with empty tables (test_buffy_filters_empty). There's no description of what the test is supposed to ensure, so I'm a bit puzzled about how to change the test.

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 a pull request may close this issue.

2 participants