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

Package redesign - future of math submodule #90

Open
eduardo-rodrigues opened this issue May 23, 2019 · 11 comments
Open

Package redesign - future of math submodule #90

eduardo-rodrigues opened this issue May 23, 2019 · 11 comments

Comments

@eduardo-rodrigues
Copy link
Member

As part of a major redesign, see #86, we need to discuss where the math submodule could/should go. The module contains classes to deal with vectors, Lorentz vectors, functions relevant toe kinematics, etc.

We've had discussions on a package for a fast Pythonic implementation of vector-related classes, so that's 100% connected with this PR. And we want to move forward having those implementations in sync with what is in uproot-methods. CC for now @henryiii and @jpivarski.
To be continued ...

@eduardo-rodrigues
Copy link
Member Author

Important note: this submodule is used by at least the https://github.com/diana-hep/madminer package, and we should keep the authors in the loop of the design going on here, to avoid unnecessary disruptions. FYI @johannbrehmer.

@johannbrehmer
Copy link

Thanks! A move is great, but if would be awesome if you leave the LorentzVector API intact :)

Let me elaborate a little bit on this. MadMiner makes heavy use of LorentzVector, not just internally, but user-facing. For instance, MadMiner users can define observables or selection cuts through expressions or functions that use LorentzVector instances.

So if LorentzVector moves to a different package, that's no problem at all -- but if the LorentzVector API changes, that could break existing code that uses MadMiner. (In that case we'll have to fix the dependency to be skhep v0.5.* for the time being).

Thanks a lot for listening to us!
Johann

@eduardo-rodrigues
Copy link
Member Author

I know we have mentioned changes but, if any, changes to the API will be really small, and probably not in the kind of methods you use. Also, we could even imagine to keep, for at least the first release as new package, a duplication of functionality with the old signatures, if better.

We will update this as the job gets done :-).

@eduardo-rodrigues
Copy link
Member Author

Hi @johannbrehmer, I think it is time to get back to this now that the vector package has been released, see https://github.com/scikit-hep/vector. Maybe you are using it already? It would be great to discuss with you if vector provides what you want, if you have feature requests, feedback on the API, and finally, how to retire what math we have here now that vector is available.

CC'ing @henryiii and @jpivarski. Feel free to add anyone relevant to the discussions.

@eduardo-rodrigues
Copy link
Member Author

As you can see from #140 I'm adding vector as a dependency to this metapackage so that it is readily available together with the rest, see for example the most recent release https://github.com/scikit-hep/scikit-hep/releases/tag/v3.1.0.

@eduardo-rodrigues
Copy link
Member Author

Hello @Saransh-cpp, I see that you remain super active in the Vector package. This metapackage could be seen as a predecessor of Vector for various things in Scikit-HEP, until we evolved for a better and more modular vision. As you can see from this task and the repo, at this stage only the math submodule remains, aside what really is otherwise a metapackage. Could you kindly have a look at the math module to identify if anything is not superseded by Vector at this point? If the case then I can remove math and finally close this very old task. Let me know if you can help. Thanks!

@Saransh-cpp
Copy link

Hi @eduardo-rodrigues, thanks for the tag! I am planning to work on this issue this week.

@Saransh-cpp
Copy link

Hi @eduardo-rodrigues!

I discussed this issue with @jpivarski and it should be okay to remove the math submodule in this repository. The module has some functionalities that vector lacks but there are no plans to implement them in vector (intentionally). Furthermore, given that the module is not actively maintained, it might have unnoticed bugs in it and we should not recommend users to use the module.

@eduardo-rodrigues
Copy link
Member Author

Thank you both.

Just to be clear, did you also look at things such as the math/isclose.py and math/numeric.py modules? Nothing interesting to port there? I was also worried that we would look what we have in math/geometry.py for lines and plans.

@jpivarski
Copy link
Member

isclose is implemented by Awkward with a NEP-18 override (so np.isclose on an Awkward Array works), and np.nextafter (like next_double) is a NumPy ufunc, so it automatically works through NEP-13.

Vector doesn't have the equivalent of extended objects like lines and planes. It seems like a natural extension, but it's one that has never been requested. It's the sort of thing you'd likely need for a detector geometry description—and in fact, a detector geometry would need much more than just lines and planes—for which there are other Python libraries, to work with geometry in Geant4's XML language. (There was a talk on that at one of the PyHEPs, I think.)

@eduardo-rodrigues
Copy link
Member Author

Ah, nice, I had not appreciated that. Good to know.

Indeed lines and planes are a bit on the edge, beyond the strict scope of Vector and more into what you would need for geometry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants