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

Let xedocs to handle avg seg and seg partitioning #1371

Merged
merged 4 commits into from Apr 30, 2024

Conversation

GiovanniVolta
Copy link
Contributor

@GiovanniVolta GiovanniVolta commented Apr 24, 2024

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

Changed how to handle the TPC partition for AB and CD SEG and avg SEG.

Relevant for xedocs PR#120 and correction PR#273

Can you briefly describe how it works?

The partition AB is distinguished from CD based on a linear and a radial cut on the (xy) plane. For the two partitions, we have different average SE gains.

We have add two schemas, one for the definition of AB/CD and one for the avg_se_gain. Here they are implemeneted.

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

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?

@GiovanniVolta
Copy link
Contributor Author

GiovanniVolta commented Apr 24, 2024

Hey @yuema137 and @dachengx,
do you have a suggestion on verifying that nothing changed for SR0? From the test performed in correction PR#274 the SR0 values of avg SEG and SEg partitioning are good!

@dachengx dachengx changed the title let xedocs to handle avg seg and seg partitioning Let xedocs to handle avg seg and seg partitioning Apr 25, 2024
@yuema137
Copy link
Collaborator

@GiovanniVolta Thanks for the PR! It looks fine for me as the dates and values are correct. But I'm wondering should we also replace the avg SEG? For now only the partitions are replaced.

Copy link
Collaborator

@yuema137 yuema137 left a comment

Choose a reason for hiding this comment

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

Another comment: I understand that the tests may fail because of not updated xedocs and other relevant repos. Could you provide the dependencies between them and point out which PRs we should use to test together? Thanks a lot

@GiovanniVolta
Copy link
Contributor Author

For the following test you have to be on correction seg_partition branch and on xedocs se_gain_partition_schema branch

immagine
immagine
immagine
immagine

Note that for these test I have specified where to fetch the data. So basically, this is what the new global version will look like.

I have also tested the online context, and it works well for the partitioning definition. For AB and CD values, we don't have to worry since it is handled by the plugin itself here (this should reply to your first question @yuema137)

immagine

@GiovanniVolta
Copy link
Contributor Author

GiovanniVolta commented Apr 26, 2024

Another comment: I understand that the tests may fail because of not updated xedocs and other relevant repos. Could you provide the dependencies between them and point out which PRs we should use to test together? Thanks a lot

Thank you, @yuema137, for the comment. So these PR should be tested with correction seg_partition branch and on xedocs se_gain_partition_schema
Actually, I think now it does not matter anymore since I have changed the default version of single_electron_gain_partition
Mmmm not sure what it is going on
Sorry I forgot to push a commit. My bad

@LuisSanchez25
Copy link
Contributor

I thought we discussed yesterday that github does not have access to our mongo database so the URLs could not be set to xedocs?

@coveralls
Copy link

coveralls commented Apr 26, 2024

Coverage Status

coverage: 91.537% (+0.02%) from 91.517%
when pulling 8b9991c on avg_seg_and_seg_partitioning
into e555c7d on master.

@GiovanniVolta GiovanniVolta marked this pull request as ready for review April 26, 2024 15:44
@yuema137
Copy link
Collaborator

@GiovanniVolta Thanks for the explanation. So, in my understanding, the `single_electron_gain_partition' config already contains the SEG values.

single_electron_gain_partition = straxen.URLConfig(

In that case, to avoid confusion, should we remove those two configs? It seems that they are set by CMT

avg_se_gain = straxen.URLConfig(

se_gain = straxen.URLConfig(

The current practice looks like a mixture of CMT and xedocs which is not ideal. So could you look into this? Thanks a lot

@GiovanniVolta
Copy link
Contributor Author

Hey @yuema137,
No, no, the single_electron_gain_partition now only takes care of linear and circular selection for defining the AB and CD partition. Similarly, the avg_se_gain and se_gain take care only of their respective variables. There is no nested indexing anymore. Sorry for the confusion. Please have a look at the correction PR#273 to further understand the two new schemas.
I have also updated the PR description. My bad; I should have done this before.

@GiovanniVolta
Copy link
Contributor Author

@flammhead asked me if these changes and the release of a new version of xedocs will affect the data lineage. My naive answer is only above the event level. The Strax/staxen version has not been changed, and the lineage does not care about the xedocs version.
@yuema137 , @dachengx is this correct?

@GiovanniVolta
Copy link
Contributor Author

The relevant PRs in xedocs and correction have been merged! We are good to go imo

Copy link
Collaborator

@yuema137 yuema137 left a comment

Choose a reason for hiding this comment

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

Thanks @GiovanniVolta ! Now it's clear to me and I think we can go ahead to merge.

@yuema137 yuema137 merged commit df21ba2 into master Apr 30, 2024
8 checks passed
@yuema137 yuema137 deleted the avg_seg_and_seg_partitioning branch April 30, 2024 17:52
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.

None yet

4 participants