DecayLanguage compatibility

[simonthor]

Solves https://github.com/scikit-hep/decaylanguage/issues/86. See this issue for further information.

The code works and all tests pass on Linux. Tasks left to do:

• [x] Add a way of installing the required packages for the fulldecay part (e.g., make it possible to run pip install phasespace[fulldecay])
• [x] Rename fulldecay to something else?
• [x] Add example use cases and documentation. Where should this be put?

[eduardo-rodrigues]

Great to see this PR! Can you explicitly link to DecayLanguage's task https://github.com/scikit-hep/decaylanguage/issues/86 in the description header since this is addressing it?

[jonas-eschle]

For the docs, I would suggest to create a jupyter notebook with a good example, this can then be converted into a static html page (such as done in zfit for example).

I suggest to create a notebook (in docs is fine) that explains and introduces the feature

[jonas-eschle]

The notebook looks nice, that's pretty much along the line of what I had in mind! Adding an example with a different mass shape and that's complete I would say

I have also added a pre-commit hook that cleans the jupyter notebooks output, so that the git repo won't be filled up too much (do you know pre-commits and how to use them locally?)

[simonthor]

I did try to use pre-commit locally but I think I got some error with my python version (might have been a different error) and did not look too deeply into trying to fix it, as the CI applies pre-commit on the code anyway. I will take a look at it again to see if I can fix it!

[simonthor]

I have now written the jupyter notebook that will serve as documentation. Should I copy the notbeook documentation to the docstrings of e.g., FullDecay.from_dict or is it enough to simply link to the notebook?

I also renamed the files to fromdecay, though not the class name since I personally feel like FromDecay seems like a strange class name. If you think that class name works, I can rename it as well.

As a side note, I managed to get pre-commit working locally.

[eduardo-rodrigues]

I have now written the jupyter notebook that will serve as documentation. Should I copy the notbeook documentation to the docstrings of e.g., FullDecay.from_dict or is it enough to simply link to the notebook?

I would still have at least one example in the method, yes, because that's what people are likely to look at via help.

I also renamed the files to fromdecay, though not the class name since I personally feel like FromDecay seems like a strange class name. If you think that class name works, I can rename it as well.

Why not call the notebood FullDecay since it is basically about that class? fromdecay is not so clear.

As a side note, I managed to get pre-commit working locally.

Not sure what is happening but you seem to have committed a huge number of files with basically no change. Is that because of the pre-commit? I would then personally do this after this MR as now it is rather difficult to find the actual files of this MR among the 50-ish touched.

[simonthor]

I would still have at least one example in the method, yes, because that's what people are likely to look at via help.

That makes sense. I will add some documentation in the docstrings as well then.

Why not call the notebood FullDecay since it is basically about that class? fromdecay is not so clear.

That could work as well. I am not entirely satisfied with the FullDecay name though, as I mainly named the class that as a place-holder. Is there any common terminology that describes a decay with multiple different ways for a particle to decay? If you think this name works, it can of course be kept the way it is.

Not sure what is happening but you seem to have committed a huge number of files with basically no change. Is that because of the pre-commit? I would then personally do this after this MR as now it is rather difficult to find the actual files of this MR among the 50-ish touched.

This was a mistake on my side, sorry! I think it is just changing the newline character on every line, as I switched from using git on Windows to git on WSL, as Linux uses different new line characters compared to Windows. I will revert the commit and only stage and commit the relevant files.

[eduardo-rodrigues]

Is there any common terminology that describes a decay with multiple different ways for a particle to decay? If you think this name works, it can of course be kept the way it is.

Not really. That's basically the (sub)set of decay chains/paths for a given mother particle. One could go for MultiDecayChains? As you know I use DecayChain in DecayLanguage to describe a single decay chain/path.

[jonas-eschle]

MultiDecayChains?

Yes, better I agree, something that expresses what it is, not what it does. What about GenMultiDecay (as an analogy to GenParticle?)

Yes, the notebook can be linked. Once I get the docs up again, we can put it on the website

[eduardo-rodrigues]

MultiDecayChains?

Yes, better I agree, something that expresses what it is, not what it does. What about GenMultiDecay (as an analogy to GenParticle?)

That's closer to the existing class hence better, yes 👍.

[simonthor]

GenMultiDecay sounds like a nice name!

Unfortunately I have just entered the exam period, which will last until the end of October. I might therefore not have time to add the final changes, i.e., change the name of the class and add the final docstrings, until the beginning of November. I hope that is ok.

In the meantime (or after), could you please review the change I made to setup.cfg (see f5da87c67ee150347fe7b3064b02d48733b0cfc1)? I have not used setuptools before so I might have used incorrect syntax when trying to make it possible to pip install phasespace[fromdecay].

[eduardo-rodrigues]

Unfortunately I have just entered the exam period, which will last until the end of October. I might therefore not have time to add the final changes, i.e., change the name of the class and add the final docstrings, until the beginning of November. I hope that is ok.

No worries, Good luck!

In the meantime (or after), could you please review the change I made to setup.cfg (see f5da87c)? I have not used setuptools before so I might have used incorrect syntax when trying to make it possible to pip install phasespace[fromdecay].

Looks just fine to me, meaning it is correct. This being said I have 2 remarks:

• I updated the dependency on Particle to avoid old versions.
• More importantly I would be surprised if somebody uses this new module without DecayLanguage itself. You only require Particle. That is correct for what the code here demands but in practice I would expect anyone using this new module to also have/use DecayLangue for the definition of the inout dict and/or some visualisation. In other words, it would seem good to me to instead requre decaylanguage >= 0.12.0.

[simonthor]

I have now renamed the class and added some examples to the docstring of GenMultiDecay.from_dict. In the docstring I tried adding a link to the notebook tutorial. However, since the tutorial of course is not available in readthedocs yet, I just added a placeholder URL. Which URL should this be changed to?

[eduardo-rodrigues]

Hi. You can guess the URL either from the direct link https://github.com/zfit/phasespace/tree/master/docs or from files under the same directory as in https://phasespace.readthedocs.io/en/stable/contributing.html. Seems a reasonable guess.

In any case, can you deal with the easy warnings from Codacy?

Shall we try and finalise this week with a first version? Is that feasible?

[eduardo-rodrigues]

(Out of curiosity, do you see as yourself the suggestions as it is then trivial to accept them one-by-one or in a batch? I ask since it seems you are doing the same changes manually.)

[simonthor]

(Out of curiosity, do you see as yourself the suggestions as it is then trivial to accept them one-by-one or in a batch? I ask since it seems you are doing the same changes manually.)

I do indeed see the suggestions and accepted most of them. For "decaylanguage" -> "DecayLanguage" however, I did a search in my IDE for all instances of "decaylanguage" to make sure that I found all of them, so that is why I made those changes manyally.

[simonthor]

I have now fixed most of the issues and suggested changes.

Regarding the code quality, Codacy complains about "1 blank line required between summary line and description (found 0)" even though I have 1 blank line between the summary and the rest of the docstring. Any idea on how to resolve this? Codacy also seems to ignore the #noqa: F401 comments so I am not sure how to resolve that.

[eduardo-rodrigues]

Let us know when ready, un-WIP it and we can do a final pass for this first version 👍.

[simonthor]

Ok, I will remove the "WIP". Besides the errors that are left in Codacy, which either contradict pre-commit hooks or complains about the code even though it looks like it follows their guidelines to me, I think this is ready.

[jonas-eschle]

Codacy errors are perfectly fine, they are accepted. We could actually disable the check.

See it as an idea, but while there is already a large agreement on coding stiles (and your code is perfectly in line with what the community agrees on), there are 2 % where the community doesn't agree. Don't bother!

[jonas-eschle]

Yep, I agree! Just figuring out how to successfully also run the notebook, there is some weird issue with graphviz

[eduardo-rodrigues]

Yep, I agree! Just figuring out how to successfully also run the notebook, there is some weird issue with graphviz

Ah, if it is for conda then you need python-graphviz, see https://github.com/conda-forge/decaylanguage-feedstock/blob/master/recipe/meta.yaml#L26.

[jonas-eschle]

So I didn't figure out the CI failures yet fully, but it is not related to the PR directly but rather that some software packages play weird together. I am still checking it

[eduardo-rodrigues]

So I didn't figure out the CI failures yet fully, but it is not related to the PR directly but rather that some software packages play weird together. I am still checking it

Sorry to hear about the pain. For Python 3.6 I had a speedy look just now and see E ImportError: This version of TensorFlow Probability requires TensorFlow version >= 2.7; Detected an installation of version 2.6.2. Please upgrade TensorFlow to proceed. So that's either a trivial update or a dependency nightmare. In any case, given the scope of this package it might not be a problem to any user if you drop 3.6 support at this stage ;-).

[jonas-eschle]

Yeah, Python 3.6 makes sense, thanks! That is a more trivial one (also, we should soon remove support for it I think), the others are just weird... and I can't reproduce it locally. Still trying, and maybe there is also gonna be a magical fix soon somewhere :D

[eduardo-rodrigues]

This is really annoying. Looking online for the error I got to https://github.com/tensorflow/tensorflow/issues/51592. Does it help you?

[jonas-eschle]

Yeaish, that's what I suspect, so I hope it may gets fixed. But then I cannot reproduce that locally, which is rather weird. Version mismatches should not really happen, or if, then it's maybe just because a new version was released newly and now packages react.

I'll give it a week or so more, and if that doesn't resolve it (TF2.7 was just resently released), I'll dig more into it.

Thanks though for the help!

[jonas-eschle]

Since we dropped Python 3.6, this is now surely a go! Many thanks @simonthor and @eduardo-rodrigues for getting this in!

[simonthor]

Thank you both as well for the guidance throughout this process!