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

z coordinate update #535

Merged
merged 8 commits into from Jun 10, 2021
Merged

z coordinate update #535

merged 8 commits into from Jun 10, 2021

Conversation

jingqiangye
Copy link
Contributor

@jingqiangye jingqiangye commented Jun 5, 2021

What does the code in this PR do / what does it improve?

  • Take electron_drift_time_gate into account when calculating z_naive
  • Fixing issue #527

Can you briefly describe how it works?

Can you give a minimal working example (or illustrate with a figure)?

A test is done in
https://xenonnt.slack.com/archives/C020GRDUNN8/p1622855301162000. Not sure why it fails automatic checks. Is it related to the Travis shutdown?

Please include the following if applicable:

  • Update the docstring(s)
  • Update the documentation
  • Tests to check the (new) code is working as desired.
  • Does it solve one of the open issues on github?

strax.Option(
name='electron_drift_time_gate',
help='Electron drift time from the gate in ns',
default=("electron_drift_time_gate", "ONLINE", True)
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
C812 missing trailing comma

@@ -375,6 +382,7 @@ def setup(self):

is_CMT = isinstance(self.config['fdc_map'], tuple)
self.electron_drift_velocity = get_correction_from_cmt(self.run_id, self.config['electron_drift_velocity'])
self.electron_drift_time_gate = get_correction_from_cmt(self.run_id, self.config['electron_drift_time_gate'])
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E501 line too long (117 > 100 characters)

@@ -395,7 +403,7 @@ def compute(self, events):
result = {'time': events['time'],
'endtime': strax.endtime(events)}

z_obs = - self.electron_drift_velocity * events['drift_time']
z_obs = - self.electron_drift_velocity * (events['drift_time'] - self.electron_drift_time_gate)
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E501 line too long (103 > 100 characters)

@@ -420,7 +431,9 @@ def compute(self, events):
'r_field_distortion_correction': delta_r,
'theta': np.arctan2(orig_pos[:, 1], orig_pos[:, 0]),
'z_naive': z_obs,
'z': z_cor})
'z': z_obs,
'z_field_distortion_correction': delta_z
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
C815 missing trailing comma in Python 3.5+

@jingqiangye jingqiangye marked this pull request as ready for review June 5, 2021 01:08
@l-althueser
Copy link
Member

We need to change the description for the z variable. Otherwise it looks good :)

@JoranAngevaare
Copy link
Contributor

Hi @jingqiangye , thanks!

  • The tests fail because some tests (py3.6) do not have access to CMT database, if we can upload a placeholder dummy value here, they should pass.
  • The tests with database access (coveralls, py3.8 etc) fail because this implementation did not take into account xenon1t. So perhaps its good if we upload the 1T value similar as we do for the drift velocitiy

@jingqiangye jingqiangye linked an issue Jun 5, 2021 that may be closed by this pull request
@jingqiangye jingqiangye marked this pull request as draft June 5, 2021 12:17
@jingqiangye
Copy link
Contributor Author

Hi @jingqiangye , thanks!

  • The tests fail because some tests (py3.6) do not have access to CMT database, if we can upload a placeholder dummy value here, they should pass.
  • The tests with database access (coveralls, py3.8 etc) fail because this implementation did not take into account xenon1t. So perhaps its good if we upload the 1T value similar as we do for the drift velocitiy

Thanks for the explanation and suggestion! I added the placeholder for electron_drift_time_gate in 1T and nT config, but it still fails the check. Does one need to add dummy values for many other parameters? (1T config has much fewer params) Or maybe I overlooked smth else?

@jingqiangye jingqiangye marked this pull request as ready for review June 7, 2021 14:32
@@ -272,6 +272,7 @@ def xenonnt_simulation(output_folder='./strax_data'):
electron_drift_velocity=("electron_drift_velocity_constant", 1.3325e-4, False),
event_info_function='disabled',
max_drift_length=96.9,
electron_drift_time_gate=("electron_drift_time_gate_constant", 1700),
Copy link
Contributor

Choose a reason for hiding this comment

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

@jingqiangye do you happen to know the correct value for 1T?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think it is the electron_drift_time_gate_constant for 1T, see Tab. 1. Is that your question? I feel like you are asking smth else... :)

Copy link
Contributor

@ershockley ershockley left a comment

Choose a reason for hiding this comment

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

@jingqiangye @jorana looks good. Can we merge this guy?

@jingqiangye
Copy link
Contributor Author

@ershockley Yeah I think so

@JoranAngevaare JoranAngevaare merged commit db4926a into master Jun 10, 2021
@JoranAngevaare JoranAngevaare deleted the gate_drift_time branch June 10, 2021 13:23
@JoranAngevaare
Copy link
Contributor

Looks good 😄

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.

Follow the FDC updates
4 participants