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

Add Corrections minitree #109

Merged
merged 16 commits into from
Jun 16, 2017
Merged

Add Corrections minitree #109

merged 16 commits into from
Jun 16, 2017

Conversation

adambrown1
Copy link
Contributor

This adds a minitree "Corrections" which provides the corrected S2 area (total, top and bottom) and the observed (uncorrected) interaction positions for the main interaction. It is envisaged to move all high-level corrections for which there is no good reason to compute during processing with pax here eventually.

Important caveat: the corrected, total S2 area is called 'cs2' just like in the Basics minitree. This could cause some problems since in Basics the S2 correction is not applied correctly (due to a pax bug). Therefore to get the correction applied correctly, while the key is duplicated, Corrections has to come before Basics in the list of treemakers passed to hax.minitrees.load:
hax.minitrees.load(datasets, ['Corrections', 'Basics'])
not
hax.minitrees.load(datasets, [ 'Basics','Corrections'])

The map to use for the S2 correction is chosen based on the run number (change at the end of science run 0). The position reconstruction correction (which is undone to give the uncorrected positions) comes from pax.

This treemaker fixes all the problems (that I know about) related to applying the S2 correction and I have checked that the correction applied using it within hax looks reasonable.

@adambrown1 adambrown1 requested a review from coderdj June 9, 2017 14:33
@adambrown1
Copy link
Contributor Author

This fixes issue #103 and is an alternative to pull request #101

@coderdj
Copy link
Contributor

coderdj commented Jun 14, 2017

This looks pretty great already. I think it raises a rather fundamental question though. This is 'how do we keep track of corrections versions?' The way this is implemented corrections versions would follow global hax versions. Honestly I don't see a problem with that but curious if others have opinions.

So if we go this way we probably can the DB-based correction for everything except gains and NN-weights (and any other gain/posrec/otherwise fundamental corrections that come up later). All energy-based corrections would be rolled into this treemaker and its version would get bumped every time there's a new correction file.

Another point: once we get all corrections in hax that are going there, do we want to drop them from pax? So then cs1, cs2, etc would exist only in this minitree and not in Basics.

@JelleAalbers
Copy link
Contributor

You would have to increment this minitree's version to bump the corrections, the hax global version does nothing (though it is being tracked during minitree creation, and there is an option to set a minimum requirement on it). But indeed, using this would move to a global correction version.

One problem is the corrections for non-standard interactions, e.g. for double scatter and delayed decay analyses. The reason we made the interaction concept in pax was to compute such corrections for several interaction pairs. If we now move corrections into hax there's a risk of duplicating code between this minitree and double scatter / delayed decay analysis minitrees.

Copy link
Contributor

@coderdj coderdj left a comment

Choose a reason for hiding this comment

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

Approved. Comment above.

@pdeperio
Copy link
Contributor

@JelleAalbers so even if hax global version changes, minitrees will not be regenerated if the minitree version did not change?

@JelleAalbers
Copy link
Contributor

Exactly. This could be changed of course, but at the moment there is no automatic process to regenerate minitrees so they are accessible for everyone. Imagine what happened this week with the Extended minitree multplied to all minitrees at once...

@coderdj
Copy link
Contributor

coderdj commented Jun 14, 2017

For versioning that's exactly the behavior we want. You don't want to change something totally different then have your minutes minitrees all recreated just because version was bumped.

For non-standard interactions I think we should just make the corrections here as well. The benefit of saving the time and effort of reprocessing is worth the extra complexity of putting it here. Maybe we can split the actual correction application into a more general function that could be applied to any interaction?

Copy link
Member

@feigaodm feigaodm left a comment

Choose a reason for hiding this comment

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

Thanks for the nice tree maker, future corrections in S1 and FDC should and will be applied here as well.

@adambrown1 adambrown1 closed this Jun 16, 2017
@adambrown1 adambrown1 reopened this Jun 16, 2017
@feigaodm feigaodm merged commit b9681bb into master Jun 16, 2017
@feigaodm feigaodm deleted the corrections_minitree branch June 16, 2017 15:27
result['x_observed'] = x_observed
result['y_observed'] = y_observed

result['cs2'] = s2.area * s2_correction
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this ensure cs2 = cs2_top + cs2_bottom? i.e. if you have top and bottom maps, why not use them both and then sum for total? should get better resolution this way, right?

@feigaodm feigaodm added this to the v1.4.5 milestone Jun 19, 2017
pdeperio added a commit to XENON1T/cax that referenced this pull request Jun 30, 2017
pietrodigangi added a commit to XENON1T/processing that referenced this pull request Jun 30, 2017
'Corrections' must precede 'Basics', according to XENON1T/hax#109
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

5 participants