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

Basic flasher functionality #3

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Basic flasher functionality #3

wants to merge 4 commits into from

Conversation

SheridanLloyd
Copy link

Creates a flasher pulse with illumination and pulse width with optional error, I also
attach some plots I generated, where do you want the multiple flash test harness code to go?

multiple_flashes_no_err
multiple_flashes_with_err

Creates a flasher pulse with illumination and pulse width with optional error
@SheridanLloyd SheridanLloyd marked this pull request as ready for review March 20, 2021 14:45
@watsonjj
Copy link
Contributor

Thank you for this contribution. I have a few questions/suggestions:

  • Could you describe the difference between flasher_pulse_width and pulse_width_err? Could these be the same parameter?
  • This is very similar to the implementation to the get_uniform_illumination method. Could we consider extending the functionality of that method so that it could also be used for the flasher investigations? (e.g. including the illumination_err argument)
  • Whatever implementation is added, some unittests should be added to ensure/enforce the new functionality.

@SheridanLloyd
Copy link
Author

Many thanks Jason for that quick review, good stuff

  • flasher_pulse_width is the the pulse width without error (e.g 4.5 ns) whereas the pulse_width_err is a percentage err applied to flasher_pulse_width and quoted separately in nextcloud flasher docs/slides (3.5 %), yes these could be merged to one param pulse_width_with_err or similar, I will do that

  • Yes, this is just get_uniform_illumination with some errors added, I can just put this into get_uniform_illumination instead and scrub the sep flasher method, will do that

  • I have a test harness but was unsure where you preferred that to be in the hierarchy e.g under event\tests or somewhere else, its different from the pytest stuff that is there in that it plots (rather than asserting anything), suggest put under event\tests unless it interferes with any auto test scripting you have

  • cool Sheridan

@watsonjj
Copy link
Contributor

It wasn't necessary to close this PR - you can just add commits to the same branch.

flasher_pulse_width is the the pulse width without error (e.g 4.5 ns) whereas the pulse_width_err is a percentage err applied to flasher_pulse_width and quoted separately in nextcloud flasher docs/slides (3.5 %), yes these could be merged to one param pulse_width_with_err or similar, I will do that

Ah so its a fluctuation on the pulse width. Then it makes sense to be its own parameter

Yes, this is just get_uniform_illumination with some errors added, I can just put this into get_uniform_illumination instead and scrub the sep flasher method, will do that

Alternatively you could call get_uniform_illumination from inside your new method, to remove the duplicated code

I have a test harness but was unsure where you preferred that to be in the hierarchy e.g under event\tests or somewhere else, its different from the pytest stuff that is there in that it plots (rather than asserting anything), suggest put under event\tests unless it interferes with any auto test scripting you have

No I mean you should add unittests that test the functionality that you add, to the pytest stuff. For demonstration of functionality, that should be kept seperate, and could be added as a jupyter notebook in the tutorials directory

@SheridanLloyd
Copy link
Author

Yes that all looks good - the close was unintended - I added a comment and it did that

@watsonjj watsonjj reopened this Mar 22, 2021
The main methods in this flasher test harness are :
Flash_100_200_pe_nsb_no_correction() - Plots fractional error (using a moving avg on charge) vs events with no pedestal subtraction
Flash_100_200_pe_subtract_nsb_pedestal() - Same as above but subtracts avg of 20 NSB samples and plots for illuminations of 100 and 200 PE
get_nsb_pedestal(), get_fractional_err_flashers(),  PlotFractionalErr() - refactoring to separate out iteration and NSB rates / flasher params to a top level so we can drive it better without tweaking params embedded in code
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.

2 participants