-
Notifications
You must be signed in to change notification settings - Fork 40
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
Make use of detector spec consistent #29
Comments
Jason, you'll have to hold off for longer than that, this is a bad time. We need the code stable for the DM experiments. I was also doing my own revamping soon, just waiting to get past other tasks, and we could clash. |
With good use of the repository, we can simultaneously have a stable release and a work-in-progress branch with both your and my changes. Let me encourage you to commit your current and ongoing changes as a separate branch, even if you feel there's more do be done. This is the approach of most other scientific software such as ROOT, G4, etc. |
hi @jpbrodsky , please see latest commit I just made today at 18c3d09 Which I think partially addresses a small part of this. Regardless, I would like you to take a look at. I had to make these changes for LZ, as they were sitting around a month and I had missed during the last release's hectic-ness. Sorry I did not make merge request first, but shoved it in. |
hi Jason, I fear that things have gotten too entrenched exactly the way they are right now, for LZ and for LUX, and XENON probably as well, and DARWIN. It has been too many months since this issue was opened, with no progress. We can't have another overhaul for readability and flexibility. Things are a bit messy still yes, but cleaner than ever before still, and most folks have gotten used to the ordered-chaos as is. I am tagging pre-release after Memorial Day and am going through and looking to close all existing open issues. I am closing this one for now, but it does not have to be forever. If you feel strongly, we can revisit in ~6-12 mos. again, but my DM collaborators will get annoyed by me if we make more major structural changes :) We should focus on physics instead. |
The VDetector class and any user-generated subtypes hold a bunch of different detector specs. These specs are referred to inconsistently in the NESTCalc class.
Some functions (GetYields) take the electric field as an input parameter, but the EField is also part of the detector class (FitEF()).
Some functions take density as an input parameter. This is not included in the detector class at all, but many similar specs are included, such as temperature.
Meanwhile, whether the xenon is gas or liquid is controlled entirely by the detector spec class (get_inGas()), rather than as an input. As a result, even low-level NESTCalc functions like GetYield(...) require a detector class to be instantiated purely to store the phase.
NESTCalc::SetDensity(temperature, pressure) takes temperature/pressure as inputs, but these are available in the detector class. Same with SetDriftVelocity.
Hopefully in a month or two I'll have some time to make a branch with a lot of specific suggestions on how to revamp the core NEST code for readability and flexibility, which could include changes that address the question of how exactly we want to use the detector class, but I'm posting this issue for now to note down some of the goals I'll have then.
The text was updated successfully, but these errors were encountered: