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!: Rename ActsSymMatrix -> ActsSquareMatrix #2387

Merged
merged 5 commits into from
Aug 22, 2023

Conversation

felix-russo
Copy link
Contributor

A square matrix has the same number of rows and columns.
A symmetric matrix is a square matrix with transposition symmetry, i.e., A.transpose() == A.

Since we don't impose transposition symmetry when initializing an ActsSymMatrix, I propose to rename it ActsSquareMatrix.

@github-actions github-actions bot added Component - Core Affects the Core module Component - Fatras Affects the Fatras module Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins Event Data Model Vertexing Track Fitting labels Aug 22, 2023
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #2387 (95ffd44) into main (c3f99be) will decrease coverage by 0.01%.
The diff coverage is 22.44%.

@@            Coverage Diff             @@
##             main    #2387      +/-   ##
==========================================
- Coverage   49.70%   49.70%   -0.01%     
==========================================
  Files         452      452              
  Lines       25467    25468       +1     
  Branches    11650    11651       +1     
==========================================
  Hits        12658    12658              
  Misses       4578     4578              
- Partials     8231     8232       +1     
Files Changed Coverage Δ
...ude/Acts/EventData/GenericBoundTrackParameters.hpp 67.10% <ø> (ø)
...ts/EventData/GenericCurvilinearTrackParameters.hpp 44.00% <ø> (ø)
...lude/Acts/EventData/GenericFreeTrackParameters.hpp 53.84% <ø> (ø)
Core/include/Acts/EventData/Measurement.hpp 80.00% <ø> (ø)
...s/EventData/MultiComponentBoundTrackParameters.hpp 61.81% <0.00%> (ø)
...Data/detail/CorrectedTransformationFreeToBound.hpp 100.00% <ø> (ø)
Core/include/Acts/Propagator/AtlasStepper.hpp 72.19% <0.00%> (ø)
...re/include/Acts/Propagator/CovarianceTransport.hpp 0.00% <ø> (ø)
Core/include/Acts/Propagator/EigenStepper.hpp 77.55% <0.00%> (ø)
Core/include/Acts/Propagator/EigenStepper.ipp 53.52% <ø> (ø)
... and 41 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@felix-russo felix-russo marked this pull request as ready for review August 22, 2023 09:22
@benjaminhuth
Copy link
Member

Good thing! 👍 I thought about this as well a lot of times, but was to lazy to make a PR...

Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

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

Probably this should go into v29, right?

@benjaminhuth benjaminhuth added this to the v29.0.0 milestone Aug 22, 2023
@felix-russo
Copy link
Contributor Author

Probably this should go into v29, right?

I would say so, yes!

@felix-russo felix-russo changed the title refactor!: Rename ActSymMatrix -> ActSquareMatrix refactor!: Rename ActSymMatrix -> ActsSquareMatrix Aug 22, 2023
@felix-russo felix-russo changed the title refactor!: Rename ActSymMatrix -> ActsSquareMatrix refactor!: Rename ActsSymMatrix -> ActsSquareMatrix Aug 22, 2023
Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

oh yeah that is very misleading. I think we already accumulated v29 changes on main so will apply automerge

@kodiakhq kodiakhq bot merged commit 7f67ffd into acts-project:main Aug 22, 2023
58 checks passed
@felix-russo felix-russo deleted the rename-ActsSymMatrix branch August 22, 2023 12:43
@paulgessinger
Copy link
Member

I'm going to guess that this will break a LOT of downstream code...

@andiwand
Copy link
Contributor

if so we can try to split up the work of fixing Athena

@felix-russo
Copy link
Contributor Author

I would help too of course!

@paulgessinger
Copy link
Member

Mostly I think HUGE changes like this probably need to be announced / discussed in the meeting.

@benjaminhuth
Copy link
Member

benjaminhuth commented Aug 22, 2023

Hmm it is only huge in terms of linediff, but should be solvable by a single global find-replace operation... I believe most interface changes are more annoying to clients...
But sure, should be announced.

@paulgessinger
Copy link
Member

I mean "huge" as in it breaks for almost everyone who's ever used ACTS.

@andiwand
Copy link
Contributor

should we revert this then? better sooner than later as this will conflict a lot of branches

@benjaminhuth
Copy link
Member

So my peronal opinion is not to revert it. Such changes are never painless, and I think its a good change since it expresses clearer what the type is

AJPfleger added a commit to AJPfleger/acts that referenced this pull request Aug 23, 2023
@paulgessinger paulgessinger added the Breaks Athena build This PR breaks the Athena build label Aug 29, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Oct 30, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Oct 30, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Nov 5, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Nov 6, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Nov 15, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Nov 15, 2023
wdconinc added a commit to eic/EICrecon that referenced this pull request Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaks Athena build This PR breaks the Athena build Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module Component - Plugins Affects one or more Plugins Event Data Model Seeding SP formation Track Finding Track Fitting Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants