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

rattest QOL update + centroid test #214

Merged
merged 12 commits into from
Dec 12, 2024

Conversation

JamesJieranShen
Copy link
Contributor

@JamesJieranShen JamesJieranShen commented Dec 9, 2024

This PR includes the following QOL updates to rattest:

  1. rattest.py is now installed to ${CMAKE_INSTALL_PREFIX}/bin, alongside all other install executables.
  2. tests can now define local ratdb experiments in their own directory.
  3. updated adjacent documentation.
  4. test scripts, particularly the root macro, is now included in the clang format checker scope.
  5. fitcentroid has been revived and updated. Previously it was throwing below-cherenkov-threshold electrons in water. I've replaced the detector medium to the default scintillator material. Also added a inner detector volume to dramatically speedup simulation time.
  6. remove old files that are no longer necessary, per @smnaugle 's suggestion.
  7. Added some basic ANSI coloring to rattest logs. Now the logs show up as below:
image

@smnaugle
Copy link
Contributor

smnaugle commented Dec 9, 2024

My comments are all cosmetic, once @JamesJieranShen pushes the commits that fix the tests (and the tests pass) it looks good to me. I don't know what the removed files under test and python/rattest were actually for, but they don't seem to be needed so I don't see any harm in getting rid of them. The changes to how ratdb tables can be loaded from within experiment directories is out of my expertise, so someone will have to weigh in on whether or not that's a good change.

@JamesJieranShen
Copy link
Contributor Author

heads up that actually the centroid test might fail -- there's actually a bug right now where the centroid fitter doesn't gracefully handle when total charge of the event is 0. This doesn't lead to a fail every single time, but does with certain seeds.

@smnaugle
Copy link
Contributor

smnaugle commented Dec 9, 2024

heads up that actually the centroid test might fail -- there's actually a bug right now where the centroid fitter doesn't gracefully handle when total charge of the event is 0. This doesn't lead to a fail every single time, but does with certain seeds.

We should have another PR to fix that bug that we can merge in at the same time as this PR. Are you able to do that @JamesJieranShen ?

@JamesJieranShen
Copy link
Contributor Author

heads up that actually the centroid test might fail -- there's actually a bug right now where the centroid fitter doesn't gracefully handle when total charge of the event is 0. This doesn't lead to a fail every single time, but does with certain seeds.

We should have another PR to fix that bug that we can merge in at the same time as this PR. Are you able to do that @JamesJieranShen ?

will do!

@tannerbk
Copy link
Contributor

tannerbk commented Dec 10, 2024

I don't understand the motivation for the generators selected to use in the macro. It seems like we're simulating three classes of events, which complicates understanding the output. I'd just simulate central, 5 MeV, isotropic electrons. We could have separate rattests for lower or higher energy events, if we choose.

@JamesJieranShen
Copy link
Contributor Author

I don't understand the motivation for the generators selected to use in the macro. It seems like we're simulating three classes of events, which complicates understanding the output. I'd just simulate central, 5 MeV, isotropic electrons. We could have separate rattests for lower or higher energy events, if we choose.

are you cool with just keeping the third one:

/generator/add combo gun2:point
/generator/vtx/set e- 0 0 0 0 3.5 15.0
/generator/pos/set 0 0 0

3.5-15MeV center runs? I'd think residual as a function of energy would be a useful thing.

@tannerbk
Copy link
Contributor

Sure, you'll need to update the analysis code accordingly.

@JamesJieranShen
Copy link
Contributor Author

Test updated -- now we simulated 1000 electrons, from 3.5MeV to 15 MeV. We plot the overall distribution of the position distribution, as well as a profile of the residual as a function of energy. The test currently takes quite a while to run (about 15 minutes), due to the increased stats. If this is not necessary I can drop it down to perhaps 100events.

@tannerbk
Copy link
Contributor

Looks good to me.

@smnaugle
Copy link
Contributor

LGTM as well.

@tannerbk tannerbk merged commit f850dcc into rat-pac:main Dec 12, 2024
5 checks passed
smnaugle pushed a commit to smnaugle/ratpac-two that referenced this pull request Jan 7, 2025
* ignore test outputs

* Install rattest to install/bin directly.

* Add local / absolute experiment to ratdb path, mainly for testing purposes.

* update documentation

* (attempt to) revive fitcentroid test

* add cformat to test scripts

* remove old files

* add basic ANSI color code for test output

* move template back to workflow root

* clean up fitcentroid code

* add validity guard to fitcentroid test

* update fitcentroid test to remove low energy runs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants