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

Streamlining and cleanup #24

Merged
merged 21 commits into from
Nov 15, 2024
Merged

Streamlining and cleanup #24

merged 21 commits into from
Nov 15, 2024

Conversation

szabo137
Copy link
Member

@szabo137 szabo137 commented Oct 8, 2024

With this PR, I try to streamline the discussion from #17 and #13 by reflecting this Proposal in the current implementation and documentation.

Changes made

  • the docstrings went from the implementation of specific coordinate systems to a separate file getter.jl
  • most of the docstrings got a rewrite to reflect the generic nature (e.g. by replacing four-momentum with Lorentz-vector-like)
  • restructured the references in the docs a bit
  • added a new coordinate system PxPyPzE to reflect the proposal from @grasph made in the 10-interface.md. Now it is possible to either implement x,y,z,t or px,py,pz,E.
  • renamed xyze to XYZT
  • renamed *mom* in the tests of XYZT to lvec to indicate that the functionality is not dedicated to momenta.
  • Tests are passing
  • Docs were updated and workflow is passing

Copy link

codecov bot commented Oct 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.91%. Comparing base (4430fee) to head (0eee94e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #24       +/-   ##
===========================================
+ Coverage   82.81%   93.91%   +11.10%     
===========================================
  Files           3        4        +1     
  Lines         128      148       +20     
===========================================
+ Hits          106      139       +33     
+ Misses         22        9       -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@szabo137
Copy link
Member Author

I guess this is ready for review. I would highly appreciate some feedback 🙆‍♂️

src/coordinate_systems/XYZT.jl Outdated Show resolved Hide resolved
src/coordinate_systems/XYZT.jl Outdated Show resolved Hide resolved
src/coordinate_systems/XYZT.jl Outdated Show resolved Hide resolved
src/getter.jl Outdated Show resolved Hide resolved
src/getter.jl Outdated Show resolved Hide resolved
src/getter.jl Outdated Show resolved Hide resolved
src/coordinate_systems/XYZT.jl Outdated Show resolved Hide resolved
Copy link
Member Author

@szabo137 szabo137 left a comment

Choose a reason for hiding this comment

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

addressed the remaining comment.

src/coordinate_systems/XYZT.jl Outdated Show resolved Hide resolved
@Moelf
Copy link
Member

Moelf commented Oct 22, 2024

Inf is just IEEE float there's nothing Julia about that, there could well be a reason to not use Inf in ROOT

@szabo137
Copy link
Member Author

Inf is just IEEE float there's nothing Julia about that, there could well be a reason to not use Inf in ROOT

Understood, thanks! However, is there a reason to not use Inf here? Are there some conflicts downstream, e.g. in UnRoot? If so, should we take care of it here, or better downstream, where the Inf becomes a problem?

@szabo137
Copy link
Member Author

Btw: are there any additional comments to this PR? Or is this ready for merging?

Uwe Hernandez Acosta and others added 3 commits October 22, 2024 10:45
@szabo137
Copy link
Member Author

Hi @ALL, what is the status of this? If I get an approval, I would like to merge 🙆‍♂️

@Moelf
Copy link
Member

Moelf commented Nov 15, 2024

I say we go for it and see if it works in downstream applications

@szabo137 szabo137 merged commit 46d7df1 into main Nov 15, 2024
7 checks passed
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