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

Update peak_treemakers.py #214

Merged
merged 1 commit into from
Feb 15, 2018
Merged

Update peak_treemakers.py #214

merged 1 commit into from
Feb 15, 2018

Conversation

katrinamiller
Copy link
Contributor

Update peak_treemakers.py with the SingleElectrons minitree class for SE waveform shape analysis.

@tunnell tunnell self-requested a review February 14, 2018 23:17
@tunnell
Copy link
Member

tunnell commented Feb 14, 2018

Can merge after tests pass

@feigaodm
Copy link
Member

feigaodm commented Feb 14, 2018

Hi @katrinamiller , thanks for the PR. However, I don't think it's so necessary to add this SingleElectron Treemaker since we have this Isolated Peak there. For eg, I used IsolatedPeak treemaker to do SingleElectron Analysis, see this notebook, you can simply use this hit bounds stuff in the pre-selection.

If we add it, it means we will double the disk usage as single electron should dominate the Isolated peaks.

@tunnell
Copy link
Member

tunnell commented Feb 14, 2018

It uses about half the disk space (i.e., 50 GB total). For some reason, applying the cuts after the fact to try to generate the equivalent of the SingleElectron selection doesn't give the same number of peaks as if you just ran the tree maker. Therefore, it would be useful to run this on everything to understand more what was going on.

@feigaodm did you check that you would get the same number of peaks as if you just ran the treemaker on its own?

@tunnell
Copy link
Member

tunnell commented Feb 14, 2018

(Added Fei so he has to approve too)

@tunnell
Copy link
Member

tunnell commented Feb 14, 2018

@katrinamiller can you explain a little about the issue you'd be having to fill @feigaodm in on the background.

@feigaodm
Copy link
Member

haha, no, I haven't check it myself. Original I was using the SingleElectrons Treemaker from @katrinamiller , Then I switched to this Isolated peaks since it's also very convenient. It would be nice if she can confirm if it's necessary to have this added. I don't have a strong preference :)

@tunnell
Copy link
Member

tunnell commented Feb 14, 2018

It's not 'necessary' but convenient (for analysts and debugging this) since only 50 GB. Unless it's annoying for processing? I'll ask Katrina to write a few sentences then you can comment.

@katrinamiller
Copy link
Contributor Author

hi @feigaodm - the issue i'm having is loading the dataset from the hax.paxroot.loop_over_dataset command produces a different number of peaks then when I load the minitree from hax.minitrees.load & applying the cuts externally. I need the dataframes to be the same to complete the SE waveform shape analysis.

@feigaodm
Copy link
Member

@katrinamiller Thanks for explanation but I don't understand your problem. I will just approve and @tunnell can merge it.

@tunnell tunnell merged commit c637935 into master Feb 15, 2018
@tunnell tunnell deleted the single_electrons branch February 15, 2018 00:17
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

3 participants