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

Created Lichen for ScienceRun1. Updated AmBe FV. Created NG FV #58

Merged
merged 11 commits into from
Jun 23, 2017

Conversation

jpienaar13
Copy link

Corrected definition of AmBe FV in sciencerun0 to reflect the correct source position, as mentioned by Dominic: https://xecluster.lngs.infn.it/dokuwiki/doku.php?id=xenon:xenon1t:analysis:dominick:sr1_ambe_check

Created NG FV. (Not sure if needed, but at least it's there if we do):
https://xecluster.lngs.infn.it/dokuwiki/doku.php?id=xenon:xenon1t:analysis:dominick:ng_early_look

@pdeperio pdeperio added this to the v1.0.0 milestone Jun 6, 2017
(source_position[2] - df['z']) ** 2) ** 0.5
return df

class AmBeFiducial(StringLichen):
Copy link
Contributor

@pdeperio pdeperio Jun 7, 2017

Choose a reason for hiding this comment

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

Repeated definition? (also shows up in codacy check below, please go through this)

@jpienaar13
Copy link
Author

Removed the doubled definition

pdeperio
pdeperio previously approved these changes Jun 8, 2017
@pdeperio
Copy link
Contributor

pdeperio commented Jun 8, 2017

Should the sciencerun0.py also be updated with the new AmBe FV?

Also, please click "Details" next to failed "codacy/pr" check, and fix as many as you can.

@jpienaar13
Copy link
Author

Will do. We can update sciencerun0. I see no reason why that would not be a good idea. @andrewgrahambrown any objections

@pdeperio pdeperio dismissed their stale review June 8, 2017 20:29

Should update sciencerun0 AmBe FV cut as well


S2Tails = sciencerun0.S2Tails

FiducialCylinder1T = sciencerun0.S2Tails
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo?

Copy link
Author

Choose a reason for hiding this comment

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

yes, that looks like a typo

S1PatternLikelihood(),
S2Width(),
S1MaxPMT(),
SingleElectronS2s()
Copy link
Contributor

Choose a reason for hiding this comment

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

@feigaodm: is this still needed after XENON1T/pax#576?

If not, I guess we should remove from SR0 lichen too once SR0 gets reprocessed?

@pdeperio
Copy link
Contributor

@jpienaar13 please also commit the AmBe FV SR0 fix to this branch.

Corrected wrong dependency of FiducialCylinder1T cut
@jpienaar13
Copy link
Author

@pdeperio

Do we want to keep the old incorrect volume, or just update SR0 AmBe fiducial to the correct one?

@pdeperio
Copy link
Contributor

Just update with new version number.

@jpienaar13
Copy link
Author

Done

@tunnell
Copy link
Member

tunnell commented Jun 21, 2017

@jpienaar13 I think you accidentally introduced style error

@jpienaar13
Copy link
Author

@tunnell That comment is a little cryptic.

@tunnell
Copy link
Member

tunnell commented Jun 21, 2017

@jpienaar13 do you see how it says that a check had failed? Also, @pdeperio needs to say if you satisfied the changes requested in the review.

@jpienaar13 But yeah, you see where it says "All checks have failed", then "codacy/pr", on the right is "details" that takes you here:

https://www.codacy.com/app/weiyuehuan/lax/pullRequest?prid=694964

It's just trailing white space on line 94 so tiny to fix.

@jpienaar13
Copy link
Author

@tunnell Eh, I thought I fixed that one yesterday.

@tunnell
Copy link
Member

tunnell commented Jun 21, 2017

You can just edit the file on the github website.

redundant since we added it in pax (XENON1T/pax#576)
@pdeperio
Copy link
Contributor

@tunnell seems like the codacy report doesn't show all the issues at once (they appear one at a time as you fix online). so i just did make lint locally and fixed a bunch manually.

@tunnell tunnell merged commit fa77f5d into master Jun 23, 2017
@pdeperio
Copy link
Contributor

@jpienaar13: @tunnell noticed that maybe this corrected AmBe FV should have gone into sciencerun0 instead, then inherited here. Is this correct?

@jpienaar13
Copy link
Author

That is correct yes.

@pdeperio
Copy link
Contributor

ah he opened an issue here #95

@pdeperio pdeperio deleted the ScienceRun1_lichens branch November 10, 2017 22:59
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.

5 participants