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

fix: updates from review #1130

Merged

Conversation

Saransh-cpp
Copy link
Contributor

@Saransh-cpp Saransh-cpp commented Jul 11, 2024

XRef #991

  • a new util function to register projection classes
  • do not modify the global awkward behavior
  • tests for geometric dimension errors

cc: @nsmith- @lgray

- a new util function to register projection classes
- do not modify the global awkward behavior
- tests for geometric dimension errors
@Saransh-cpp Saransh-cpp changed the title Updates from review fix: updates from review Jul 11, 2024
vector.LorentzVectorArray,
PtEtaPhiMCandidateArray, # noqa: F821
)
register_projection_classes(
Copy link
Member

Choose a reason for hiding this comment

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

Ok this is my misunderstanding, I hadn't realized each case is somewhat different so the function doesn't in the end save any code duplication. Though at least for many of these the first 3 arguments are the same

Copy link
Member

Choose a reason for hiding this comment

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

I think in this case what you had before was more readable code, is it ok to revert? No need to make separate PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

arg, sorry, I thought convo was done, my bad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have to create a separate "revert" PR to fix this. There are some changes in this PR that should go in, like the extra tests and not modifying the global awkward behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to revert the subset of changes directly on the other PR and push, or make a revert PR if you prefer

@lgray lgray merged commit 3d69b20 into CoffeaTeam:use_scikithep_vector Jul 18, 2024
4 of 6 checks passed
@Saransh-cpp Saransh-cpp deleted the use_scikithep_vector branch July 19, 2024 09:31
@Saransh-cpp Saransh-cpp restored the use_scikithep_vector branch July 19, 2024 09:32
@Saransh-cpp Saransh-cpp deleted the use_scikithep_vector branch July 22, 2024 10:12
This pull request was closed.
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