Skip to content
This repository has been archived by the owner on Jun 5, 2024. It is now read-only.

S2 optical prop #305

Merged
merged 4 commits into from
Feb 9, 2022
Merged

S2 optical prop #305

merged 4 commits into from
Feb 9, 2022

Conversation

KaraMelih
Copy link
Contributor

What is the problem / what does the code in this PR do
Samples optical propagation delays in the gas phase from a G4 simulated data.
The S2 pulse workflow is a bit changed; photon channels sampling is now independent from photon times. This was needed so that photon time delays can be added based on whether they are detected at top/bottom arrays.
!To add this effect it uses a local aux file for now, the file will be added to private_nt_aux repo!

WFSim needs you:

  • Please add a test for this PR, as a bare minimum, make sure it's covered in coveralls!
  • Can you add a docsting to all your functions?

Pay attention:

  • Due to databases being needed for testing, making a PR from your own fork will typically NOT run the tests. If you then merge master might break

@KaraMelih KaraMelih requested a review from JYangQi00 January 31, 2022 15:31
@JYangQi00
Copy link
Contributor

Hi Melih, this looks good, but how do you propose we change the config to include 'confxy'?

wfsim/core/s2.py Outdated
config=config,
resource=resource)
elif 'garfield' in config['s2_luminescence_model']:
confxy = True if 'confine distance' in config['s2_luminescence_model'] else False
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JYangQi00 Hi Jianyang currently, I added it here. So while comparing the two, I simulate once setting {'s2_luminescence_model':'garfield'} and once with {'s2_luminescence_model':'garfield confine distance'} so both confxy=True and confxy=False are callable

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are adding a parameter, does it make sense to make it float number that defines "range" of confinement? For example, negative value (i.e. -1) means that it is turned off, while positive value means that it will be confined to certain range (for example, (-value, +value)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here bee4444 it can now be set as
{'s2_luminescence_model':'garfield confxy=0.05'}
If there is no 'confxy=' string in s2_luminescence_model it uses the current default from master branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and here I uploaded a notebook, using this updates. I'm trying to compare the data vs WFSim with several configs

Copy link
Contributor

@JYangQi00 JYangQi00 left a comment

Choose a reason for hiding this comment

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

Okay, I see, looks good!

@KaraMelih KaraMelih merged commit 0611f4d into jianyang_wfsim_tests Feb 9, 2022
@KaraMelih KaraMelih deleted the s2_optical_prop branch February 9, 2022 08:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants