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

[WIP] Proposal: new implementation of LorentzVector #13

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

szabo137
Copy link
Member

@szabo137 szabo137 commented Oct 8, 2024

Description

This PR proposes a new, flexible implementation of the LorentzVector type, designed to work seamlessly with any coordinate system supported by LorentzVectorBase. This includes both the currently available coordinate systems and any future systems added to LorentzVectorBase.

Key Proposal

The new LorentzVector will automatically provide the correct accessor functions for all subtypes of LorentzVectorBase.AbstractCoordinateSystem. This approach aims to make the LorentzVectors more adaptable and interoperable with different coordinate systems.

Status

While this implementation is still a work in progress, I would appreciate your feedback on the overall concept and any suggestions for improvement.

TODOs

  • Fix unit tests: Ensure all existing unit tests pass with the new implementation.
  • Reconsider unit tests: Review the current tests and consider rewriting them to better align with the updated architecture.
  • Add converters between coordinate systems: Implement a mechanism to convert between different coordinate systems (maybe using Base.convert or just the constructor LorentzVector{CS,T}).
  • Implement arithmetic for arbitrary coordinate systems: Ensure operations work correctly across different coordinate systems.
  • Discuss correct exports: Finalize which functions and types should be exported from LorentzVectorHEP to ensure a clean and usable API.
  • remove add_LorentzVectorBase.jl and respective CI entry after Streamlining and cleanup LorentzVectorBase.jl#24 is merged and LorentzVectorBase.jl is registered

@Moelf
Copy link
Member

Moelf commented Oct 8, 2024

doesn't seem to be ready for review as tests don't pass.

we can discuss re-writing tests and changing exports whatever later, first let's get existing set of unit tests to pass I think.

@szabo137
Copy link
Member Author

doesn't seem to be ready for review as tests don't pass.

we can discuss re-writing tests and changing exports whatever later, first let's get existing set of unit tests to pass I think.

You are right, of course. This implementation also needs JuliaHEP/LorentzVectorBase.jl#24 to be merged.

I will try to find some time next week to fix the tests and do some cleanup.

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 60.74766% with 42 lines in your changes missing coverage. Please review.

Project coverage is 61.11%. Comparing base (d1bab83) to head (020ab1a).

Files with missing lines Patch % Lines
src/print.jl 0.00% 17 Missing ⚠️
src/arithmetics.jl 56.25% 14 Missing ⚠️
src/utility.jl 81.08% 7 Missing ⚠️
src/conversion.jl 72.72% 3 Missing ⚠️
src/types.jl 90.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #13       +/-   ##
===========================================
- Coverage   84.96%   61.11%   -23.86%     
===========================================
  Files           3        6        +3     
  Lines         153      108       -45     
===========================================
- Hits          130       66       -64     
- Misses         23       42       +19     

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

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.

2 participants