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

Muveto + hitlet fixes #355

Merged
merged 47 commits into from Dec 8, 2020
Merged

Muveto + hitlet fixes #355

merged 47 commits into from Dec 8, 2020

Conversation

WenzDaniel
Copy link
Collaborator

@WenzDaniel WenzDaniel commented Nov 20, 2020

What is the problem / what does the code in this PR do
This PR changes two things:

  1. It adds the possibility to change the settings ending of a child compared to the parent. Example threshold_parentending can now be changed into threshold_childending instead of threshld_parentending_childending. I also added some additional comments in the code to make things a bit easier to understandable.
  2. In this PR we are making some bug fixes to hitlets.py. The fixes address the divided by zero bug in fwxm which can occur for some irregular data. The corresponding test is also updated. Further, I restructured the function a bit for better readability/logic.

@WenzDaniel WenzDaniel marked this pull request as ready for review November 23, 2020 10:00
@JoranAngevaare
Copy link
Member

@WenzDaniel , let me know when ready. I would appreciate if you un-draft when this is the case.

Copy link
Member

@JoranAngevaare JoranAngevaare left a comment

Choose a reason for hiding this comment

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

I think I'm fine with this PR. The overwrite_parents_end feels a bit patchy and also don't know if such an option would be useful for other experiments. I think another way around would be to have the neutron veto plugins child_ends_with = _nv and have the plugin recognize that we we should replace that with _mv.

Haven't tested the code itself, let me know if you think I need to try to break it.

strax/context.py Outdated Show resolved Hide resolved
strax/processing/hitlets.py Outdated Show resolved Hide resolved
strax/processing/hitlets.py Outdated Show resolved Hide resolved
strax/processing/hitlets.py Show resolved Hide resolved
strax/processing/hitlets.py Show resolved Hide resolved
@JoranAngevaare
Copy link
Member

Thanks for the changes, haven't looked at them very detailedly but I think this will require some changes on straxen. Can you explain what we will need to change, will e.g. _he channels now need both attributes? When will you need to update which?

Also, perhaps this is good to add to the documentation.

Finally, I think the tests are also not using the convention used here. I can imagine that a deprecation-phase might make sense as otherwise everything starts breaking at once.

@WenzDaniel WenzDaniel marked this pull request as draft November 26, 2020 17:23
@WenzDaniel
Copy link
Collaborator Author

Sorry my fault I should have turned it into a draft again. You are right, it requires some more modifications also in XENONnT/straxen#287

@WenzDaniel WenzDaniel marked this pull request as ready for review December 3, 2020 14:39
@WenzDaniel WenzDaniel marked this pull request as draft December 4, 2020 07:15
@WenzDaniel WenzDaniel marked this pull request as ready for review December 4, 2020 09:54
@WenzDaniel
Copy link
Collaborator Author

Hej Joran, this morning I had an idea how to simplify the whole child_plugin business. I removed the whole thing about ends_with and replaced it with the following three things:

Plugin side:
A plugin which inherits the compute statement from another plugin has to be specified as a child plugin (child_plugin=True).
Option side:
Since we cannot change the keys of self.config['key'] in the super.compute() statement we have to overwrite the corresponding options according to the new options specified by the child-plugin. This issue is the same as before. However, this is now done slightly differently on the option side. All options of a child plugin which should overwrite the option of the parent plugin has to have the following two attributes:

  • child_option=True showing that the option is a child option which overwrites a parent
  • parent_option_name=string Name of the parent config to find it and overwrite it

Although, child_option is a redundancy I kept it since it adds an additional layer of security fort he user. The lineage of the child is the same as in previous versions. All straxen plugins have been updated accordingly in XENONnT/straxen#287.

I also updated the documentation and tests. Further, I addressed your request of adding a test for the divided by zero-error in get_fwxm.

@JoranAngevaare
Copy link
Member

Thanks Daniel! This looks much cleaner, thanks a lot! To put the icing on the cake, perhaps can you also update the https://strax.readthedocs.io/en/latest/advanced/plugin_dev.html?

I think it deserves a separate heading as it's quite different than all the other types. In fact there is no such thing as a strax.ChildPlugin. You would really make my day if you can just code a minimal working example there!

The travis failing is odd on 3.6, it claims numpy incompatibility between pandas an py36. Don't know if it will persist. Perhaps we need to fix numpy before pandas as a requirement.

@WenzDaniel
Copy link
Collaborator Author

Okay, I added an example and some more words.

Copy link
Member

@JoranAngevaare JoranAngevaare left a comment

Choose a reason for hiding this comment

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

Thanks a lot. It's really nice that you took the effort to write such a clear example.

Would you say that it makes more sense to put Plugin types before Plugin inheritance. I would be inclined to argue so but its up to you, feel free to merge.

@WenzDaniel
Copy link
Collaborator Author

I was thinking the very same to be honest :D But was not sure either. I will change it

@WenzDaniel WenzDaniel merged commit bdad527 into AxFoundation:master Dec 8, 2020
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.

Unsafe division in hitlets.py
2 participants