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

DecayLanguage compatibility #63

Merged
merged 73 commits into from
Nov 24, 2021
Merged

Conversation

simonthor
Copy link
Contributor

@simonthor simonthor commented Sep 21, 2021

Solves scikit-hep/decaylanguage#86. See this issue for further information.

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

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

@eduardo-rodrigues
Copy link
Contributor

eduardo-rodrigues commented Sep 22, 2021

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

Copy link
Contributor

@jonas-eschle jonas-eschle left a comment

Choose a reason for hiding this comment

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

Very nice and thanks a lot! I agree with Eduardos comments, additional thoughts

  • name, what about "fromdec" or "fromdecay"?
  • the docstyle in phasespace is the google one, not the numpy (see other docs)

tests/fulldecay/test_mass_functions.py Outdated Show resolved Hide resolved
phasespace/fulldecay/fulldecay.py Outdated Show resolved Hide resolved
phasespace/fulldecay/mass_functions.py Outdated Show resolved Hide resolved
tests/fulldecay/test_fulldecay.py Outdated Show resolved Hide resolved
tests/fulldecay/test_mass_functions.py Outdated Show resolved Hide resolved
tests/fulldecay/example_decay_chains.py Outdated Show resolved Hide resolved
phasespace/fulldecay/mass_functions.py Outdated Show resolved Hide resolved
phasespace/fulldecay/__init__.py Outdated Show resolved Hide resolved
phasespace/fulldecay/fulldecay.py Outdated Show resolved Hide resolved
@jonas-eschle
Copy link
Contributor

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

simonthor and others added 20 commits September 26, 2021 18:26
Also made some minor clarifications to the docstrings.
Copy link
Contributor

@eduardo-rodrigues eduardo-rodrigues 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 this is ready for a first version 👍.

@jonas-eschle
Copy link
Contributor

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

@eduardo-rodrigues
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

This is really annoying. Looking online for the error I got to tensorflow/tensorflow#51592. Does it help you?

@jonas-eschle
Copy link
Contributor

jonas-eschle commented Nov 19, 2021

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
Copy link
Contributor

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

@simonthor
Copy link
Contributor Author

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

@simonthor simonthor deleted the decay2phasespace branch November 24, 2021 22:57
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.

3 participants