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: Implement Indexed builder #2883

Merged
merged 9 commits into from
Dec 8, 2023
Merged

Conversation

zonca
Copy link
Collaborator

@zonca zonca commented Dec 8, 2023

As discussed with @jpivarski and @ManasviGoyal, here is the implementation of the Indexed layout builder.
The final objective is to use this for EnumType instead of using NumpyBuilder.

@zonca zonca self-assigned this Dec 8, 2023
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Merging #2883 (6d31acc) into main (9400780) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

I approved this on zonca#3, and see that it's the same Indexed Builder implementation here.

This doesn't have the

- cmake -B build -S header-only -DCMAKE_RUNTIME_OUTPUT_DIRECTORY=bin -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTS=ON
+ cmake -B build -S header-only -DCMAKE_RUNTIME_OUTPUT_DIRECTORY=bin -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTS=ON

update in .github/workflows/header-only-test.yml, but that's unrelated to adding the new Builder, so it's best not to have it in the PR, anyway. Everything else looks goo. (The zonca#3 was a difference after IndexedOption had been copied to Indexed, but this is a more direct addition.)

I'll merge it when the tests pass.

@jpivarski
Copy link
Member

We only need one approval for merging. Last time, @ManasviGoyal also approved it, anyway.

@jpivarski jpivarski enabled auto-merge (squash) December 8, 2023 17:20
@jpivarski jpivarski merged commit bd871a5 into scikit-hep:main Dec 8, 2023
38 checks passed
@zonca
Copy link
Collaborator Author

zonca commented Dec 8, 2023

I approved this on zonca#3, and see that it's the same Indexed Builder implementation here.

This doesn't have the

- cmake -B build -S header-only -DCMAKE_RUNTIME_OUTPUT_DIRECTORY=bin -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTS=ON
+ cmake -B build -S header-only -DCMAKE_RUNTIME_OUTPUT_DIRECTORY=bin -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTS=ON

update in .github/workflows/header-only-test.yml, but that's unrelated to adding the new Builder, so it's best not to have it in the PR, anyway. Everything else looks goo. (The zonca#3 was a difference after IndexedOption had been copied to Indexed, but this is a more direct addition.)

right, that was already merged back in #2870

@zonca zonca deleted the indexed_builder branch December 8, 2023 17:30
@jpivarski
Copy link
Member

@all-contributors please add @zonca for code

Copy link
Contributor

@jpivarski

I've put up a pull request to add @zonca! 🎉

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.

None yet

2 participants