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

refactor!: Using non const InternalSpacePoint objects #1196

Merged
merged 8 commits into from
Mar 18, 2022

Conversation

noemina
Copy link
Contributor

@noemina noemina commented Mar 17, 2022

This PR tries to fix usage of mutable properties in #1168 and #1143.

Tagging: @paulgessinger @robertlangenberg @LuisFelipeCoelho

BREAKING CHANGE: Remove assumption on constness of InternalSpacePoint.
The need of this major change is to allow seed finding and filtering to evaluate and set properties of InternalSpacePoints as soon as they are candidates for triplet formation and seed quality evaluation.

@noemina noemina added this to the next milestone Mar 17, 2022
@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #1196 (26fcf40) into main (1674bce) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1196      +/-   ##
==========================================
- Coverage   47.81%   47.79%   -0.03%     
==========================================
  Files         360      360              
  Lines       18591    18600       +9     
  Branches     8769     8770       +1     
==========================================
  Hits         8890     8890              
- Misses       3649     3658       +9     
  Partials     6052     6052              
Impacted Files Coverage Δ
Core/include/Acts/Seeding/BinnedSPGroup.hpp 0.00% <0.00%> (ø)
Core/include/Acts/Seeding/BinnedSPGroup.ipp 0.00% <0.00%> (ø)
Core/include/Acts/Seeding/InternalSeed.hpp 0.00% <ø> (ø)
Core/include/Acts/Seeding/InternalSpacePoint.hpp 0.00% <0.00%> (ø)
Core/include/Acts/Seeding/SeedFilter.hpp 0.00% <ø> (ø)
Core/include/Acts/Seeding/SeedFilter.ipp 0.00% <0.00%> (ø)
Core/include/Acts/Seeding/SeedFinderUtils.ipp 0.00% <0.00%> (ø)
Core/include/Acts/Seeding/Seedfinder.hpp 0.00% <ø> (ø)
Core/include/Acts/Seeding/SpacePointGrid.hpp 0.00% <ø> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Looks good overall. I added a few comments.

I believe this breaks backward compatibility, and should be noted. Once we're ready to merge, can you write 2 sentences describing the change, that we can put into the release notes once this goes in?

Core/include/Acts/Seeding/InternalSpacePoint.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/InternalSpacePoint.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/InternalSpacePoint.hpp Outdated Show resolved Hide resolved
@krasznaa
Copy link
Member

@beomki-yeo, @konradkusiak97, could you help Noemi update the CUDA and SYCL seedfinder code in this PR? Unfortunately I think the changes will have to be made in this single PR, as she is changing some interfaces that all implementations of the seed-finding are using. (So we can't easily just patch the CUDA and SYCL codes first, before this change would be made.)

As far as I can see the changes necessary for the CUDA and SYCL plugins should be mostly trivial. (Re)moving some const statements here and there. But I thought you two should help out with that. 😉 (By either telling Noemi what she should do in those plugins to make them work with her updates, or by sending her patch files with the changes that you would propose.)

@noemina noemina changed the title feat: Using non const InternalSpacePoint objects feat!: Using non const InternalSpacePoint objects Mar 18, 2022
@konradkusiak97
Copy link

@beomki-yeo, @konradkusiak97, could you help Noemi update the CUDA and SYCL seedfinder code in this PR? Unfortunately I think the changes will have to be made in this single PR, as she is changing some interfaces that all implementations of the seed-finding are using. (So we can't easily just patch the CUDA and SYCL codes first, before this change would be made.)

As far as I can see the changes necessary for the CUDA and SYCL plugins should be mostly trivial. (Re)moving some const statements here and there. But I thought you two should help out with that. wink (By either telling Noemi what she should do in those plugins to make them work with her updates, or by sending her patch files with the changes that you would propose.)

Okay, I made the changes for CUDA and SYCL plugins.

@beomki-yeo
Copy link
Contributor

Did you change all CUDA 1&2 plugins?

@konradkusiak97
Copy link

Did you change all CUDA 1&2 plugins?

Yes

@beomki-yeo
Copy link
Contributor

Great Thanks!

@noemina
Copy link
Contributor Author

noemina commented Mar 18, 2022

I have committed the changes from @konradkusiak97. Thank you!

@paulgessinger paulgessinger changed the title feat!: Using non const InternalSpacePoint objects refactor!: Using non const InternalSpacePoint objects Mar 18, 2022
Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Let's do this then.

@kodiakhq kodiakhq bot merged commit be5fec4 into acts-project:main Mar 18, 2022
@paulgessinger paulgessinger modified the milestones: next, v18.0.0 Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants