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

feat: add constructors for VectorObject2D and MomentumObject2D #89

Merged
merged 15 commits into from
Oct 18, 2022

Conversation

henryiii
Copy link
Member

@henryiii henryiii commented Apr 15, 2021

Closes #88.

WIP, currently just 2D Object implemented.

@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2022

Codecov Report

Base: 81.04% // Head: 81.08% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (d9a4462) compared to base (23a10c5).
Patch coverage: 78.87% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #89      +/-   ##
==========================================
+ Coverage   81.04%   81.08%   +0.03%     
==========================================
  Files          96       96              
  Lines       10624    10672      +48     
==========================================
+ Hits         8610     8653      +43     
- Misses       2014     2019       +5     
Impacted Files Coverage Δ
src/vector/backends/numpy.py 78.10% <ø> (ø)
src/vector/backends/awkward.py 51.63% <33.33%> (ø)
src/vector/_methods.py 78.60% <70.96%> (-0.10%) ⬇️
src/vector/backends/object.py 73.67% <94.11%> (+0.50%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@henryiii henryiii self-assigned this Jun 27, 2022
Comment on lines 852 to 866
def __init__(
self, azimuthal: typing.Optional[AzimuthalObject] = None, **kwargs: float
) -> None:
if not kwargs and azimuthal is not None:
self.azimuthal = azimuthal
elif kwargs and azimuthal is None:
if set(kwargs) == {"px", "py"}:
self.azimuthal = AzimuthalObjectXY(kwargs["px"], kwargs["py"])
elif set(kwargs) == {"pt", "phi"}:
self.azimuthal = AzimuthalObjectRhoPhi(kwargs["pt"], kwargs["phi"])
else:
raise TypeError("invalid arguments, must be px=, py= or pt=, phi=")
else:
raise TypeError("must give Azimuthal if not giving keyword arguments")

Copy link
Member

Choose a reason for hiding this comment

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

I went through the overloads of vector.obj and a user can indeed create a MomentumObject by specifying a mixture of momentum and generic coordinates. I think this should be removed and the momentum coordinate names must be converted to generic in the __init__ method of VectorObject classes, which will then be inherited by MomentumObject classes.

Comment on lines 688 to 697
def __init__(
self, azimuthal: typing.Optional[AzimuthalObject] = None, **kwargs: float
) -> None:
if not kwargs and azimuthal is not None:
self.azimuthal = azimuthal
elif kwargs and azimuthal is None:
if set(kwargs) == {"x", "y"}:
self.azimuthal = AzimuthalObjectXY(kwargs["x"], kwargs["y"])
elif set(kwargs) == {"rho", "phi"}:
self.azimuthal = AzimuthalObjectRhoPhi(kwargs["rho"], kwargs["phi"])
else:
raise TypeError("invalid arguments, must be x=, y= or rho=, phi=")
else:
raise TypeError("must give Azimuthal if not giving keyword arguments")
Copy link
Member

Choose a reason for hiding this comment

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

Removing the MomentumObject constructor and changing the momentum coordinates to generic ones works!

Suggested change
def __init__(
self, azimuthal: typing.Optional[AzimuthalObject] = None, **kwargs: float
) -> None:
if not kwargs and azimuthal is not None:
self.azimuthal = azimuthal
elif kwargs and azimuthal is None:
if set(kwargs) == {"x", "y"}:
self.azimuthal = AzimuthalObjectXY(kwargs["x"], kwargs["y"])
elif set(kwargs) == {"rho", "phi"}:
self.azimuthal = AzimuthalObjectRhoPhi(kwargs["rho"], kwargs["phi"])
else:
raise TypeError("invalid arguments, must be x=, y= or rho=, phi=")
else:
raise TypeError("must give Azimuthal if not giving keyword arguments")
def __init__(
self, azimuthal: typing.Optional[AzimuthalObject] = None, **kwargs: float
) -> None:
for k, v in kwargs.copy().items():
kwargs.pop(k)
kwargs[_repr_momentum_to_generic.get(k, k)] = v
if not kwargs and azimuthal is not None:
self.azimuthal = azimuthal
elif kwargs and azimuthal is None:
if set(kwargs) == {"x", "y"}:
self.azimuthal = AzimuthalObjectXY(kwargs["x"], kwargs["y"])
elif set(kwargs) == {"rho", "phi"}:
self.azimuthal = AzimuthalObjectRhoPhi(kwargs["rho"], kwargs["phi"])
else:
raise TypeError("invalid arguments, must be x=, y= or rho=, phi=")
else:
raise TypeError("must give Azimuthal if not giving keyword arguments")

Copy link
Member

Choose a reason for hiding this comment

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

A minor issue with this -

vector.obj(rho=1, phi=2)

right now constructs a VectorObject2D object.

But the user would now also be able to do -

vector.MomentumObject2D(rho=1, phi=2)

@Saransh-cpp
Copy link
Member

I think this PR should work now, given that we are fine with passing generic coordinates into MomentumObject classes. The same is under discussion for the shortcut constructors (obj here), which can be handled in another PR.

@Saransh-cpp Saransh-cpp marked this pull request as ready for review July 8, 2022 17:30
@Saransh-cpp Saransh-cpp changed the title feat: useful constructor feat: add constructor for VectorObject2D and MomentumObject2D Jul 29, 2022
@Saransh-cpp Saransh-cpp changed the title feat: add constructor for VectorObject2D and MomentumObject2D feat: add constructors for VectorObject2D and MomentumObject2D Jul 29, 2022
@Saransh-cpp Saransh-cpp added the feature New feature or request label Aug 29, 2022
@Saransh-cpp Saransh-cpp added this to the v1.0.0 milestone Aug 29, 2022
@henryiii
Copy link
Member Author

henryiii commented Oct 6, 2022

Feel free to turn of the unreachable check for mypy if it triggers too much, and 1-2 type ignores need [misc] added.

@Saransh-cpp
Copy link
Member

Done! I've also refactored out the type safety code from the obj constructor.

@henryiii
Copy link
Member Author

henryiii commented Oct 17, 2022

I can't approve "my own" (according to GitHub) PR, but I reviewed it so verbally approving here!

@Saransh-cpp
Copy link
Member

Merged! Thanks for the review! 🎉

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

Successfully merging this pull request may close these issues.

feat: new constructor
3 participants