-
Notifications
You must be signed in to change notification settings - Fork 11
Add Trude documentation #12
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
base: main
Are you sure you want to change the base?
Conversation
docs/source/trude.rst
Outdated
Baseline subtraction of SiPM waveforms | ||
:::::::::::::::::::::::::::::::::::::: | ||
|
||
Same procedure as described in :ref:`Baseline subtraction of SiPM waveforms` section of the *Irene* documentation with the option of using the mean instead of the mode of the waveform for the baseline subtraction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link does not go to Irene
's section. It is connected with the title of this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also Irene must be referenced using :doc:name_city
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I managed to fix this, let me know.
docs/source/trude.rst
Outdated
Baseline subtraction of SiPM waveforms | ||
:::::::::::::::::::::::::::::::::::::: | ||
|
||
Same procedure as described in :ref:`Baseline subtraction of SiPM waveforms` section of the *Irene* documentation with the option of using the mean instead of the mode of the waveform for the baseline subtraction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also Irene must be referenced using :doc:name_city
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I could understand everything well, most of my comments are just suggestions that may improve clarity :)
docs/source/trude.rst
Outdated
This page is currently not created, we are working on it. If you would like to contribute on this documentation, reach `me <helena.almamol@gmail.com>`_! | ||
*From Germanic, Gertrud, hypocorism: female friend.* | ||
|
||
This city produces the light and dark spectrum of SiPMs for dedicated calibration runs. This is achieved by selecting regions in the SiPM waveforms where LED pulses are expected and regions which end 2 microseconds before, respectively and integrating the content within each region. The regions before LED pulses should only contain electronics noise and dark counts giving the zero external light approximation whereas those in time with the pulses will contain one or more detected photoelectrons. The waveform integrals are split into two groups: those with expected photoelectrons (light) and those without expected photoelectrons (dark). Each group produces a different spectrum. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why 2 microseconds was the chosen time before the expected region? I understand this guarantees that the two types of region don't overlap, but why was 2 microseconds chosen instead of 3 microseconds for example? If this is unimportant then you can ignore this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is it is just to avoid the overlapping of both regions. @carmenromo is more experienced in using this code, perhaps she knows a better reason for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think due to the pulse length 2 microseconds were enough to get the dark spectrum. But Andrew took the decision before we got into calibration :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it could be included on the text that this value is hardcoded and chosen to avoid overlap of both spectrums. I think it's not explained and it's a fair question.
|
||
* - ``min|max_bin`` | ||
- ``float`` | ||
- Lower/upper limit of the number of :math:`ADCs` of the waveform to be considered for the spectrum. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could specify whether this applies to light spectrum or dark spectrum or both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I didn't understand this comment, but Trude is used to compute the spectrum (light and dark) of a given pulsed waveform. Additionally, it can be used to compute just the dark spectrum if the input is a dark waveform, but it's not the main purpose. I don't find it necessary to include this clarification here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries Miryam, I think it was me who read this part wrong!
docs/source/trude.rst
Outdated
* ``/Run/runInfo`` | ||
* ``/RD/sipmrwf`` | ||
* ``/Run/events``: list of the ``evt_number`` and the correspondent ``timestamp`. | ||
* ``/Run/runInfo``: stores de ``run_number``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here de
is a typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clear and concise. All my comments have also been addressed. Well done Miryam!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked the PR, since @carmenromo is already out. Just few comments that could help on the documentation, and some beautification suggestions.
docs/source/trude.rst
Outdated
This page is currently not created, we are working on it. If you would like to contribute on this documentation, reach `me <helena.almamol@gmail.com>`_! | ||
*From Germanic, Gertrud, hypocorism: female friend.* | ||
|
||
This city produces the light and dark spectrum of SiPMs for dedicated calibration runs. This is achieved by selecting regions in the SiPM waveforms where LED pulses are expected and regions which end 2 microseconds before, respectively and integrating the content within each region. The regions before LED pulses should only contain electronics noise and dark counts giving the zero external light approximation whereas those in time with the pulses will contain one or more detected photoelectrons. The waveform integrals are split into two groups: those with expected photoelectrons (light) and those without expected photoelectrons (dark). Each group produces a different spectrum. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it could be included on the text that this value is hardcoded and chosen to avoid overlap of both spectrums. I think it's not explained and it's a fair question.
- ``int`` | ||
- Number of bins to be considered for the integral starting at ``integral_start``. With ``integral_start`` these values are ideally chosen to select the whole pulse of the LED. | ||
|
||
* - ``integrals_period`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have units? If so, I would add them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is time units, the exact unit depends on the pulse.
|
||
The light bins will be centered in the light pulse(s) and the dark bins are chosen as an interval of ``integral_width`` bins which ends 2 microseconds before the light pulse begins, as shown here: | ||
|
||
.. image:: images/trude/wf_intervals.png |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will help when reading the documentation to have a better explanation of the parameters defined before and to which range, etc on the spectra correspond. You can check Buffy documentation as a reference.
Spectrum histogram | ||
::::::::::::::::::: | ||
|
||
The last step would be the integration of the dark and light bins in order to obtain the respective spectrum histograms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as the one before. It would be good to mention which configuration parameters are used when creating the spectrum histogram.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I don't understand this comment. We use all of the config parameters listed above.
I added a couple more things to the documentation (including plots and changes in already existing plots). @joshgrocott feel free to take a general look to it and see if it makes sense. Any suggestions will be welcomed :) |
docs/source/trude.rst
Outdated
|
||
* - ``integral_start`` | ||
- ``int`` | ||
- Bin where the integrals start. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We found that in Phyllis that integral_start and integral_width need to be given in microseconds, but the waveforms used were in (25 I think?) nanosecond time bins. I guess this would also be the case for trude? So would be worth mentioning in the documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, integral_start
should be given in microseconds, but it's not the case for integral_width
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment regarding time bins for integral limits
This PR adds the documentation for the city Trude.
All this info will eventually be here: https://pr012-sw-docs.readthedocs.io/en/latest/trude.html