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

Replace indptr with sparse and dense grouped index vector #402

Merged
merged 37 commits into from Oct 16, 2023

Conversation

mgovers
Copy link
Member

@mgovers mgovers commented Oct 10, 2023

Replace indptr with SparseIdxVector

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers added the improvement Improvement on internal implementation label Oct 10, 2023
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers changed the title apply indptr replace source bus (still segfaults) Replace indptr with sparse and dense grouped index vector Oct 11, 2023
…ature/refactor-replace-indptrs

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
…ature/refactor-replace-indptrs

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
…ature/refactor-replace-indptrs

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers force-pushed the feature/refactor-replace-indptrs branch from af08fb3 to 6c80ebb Compare October 12, 2023 15:02
Base automatically changed from feature/refactor-indptr to main October 12, 2023 15:33
mgovers and others added 7 commits October 12, 2023 22:50
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers marked this pull request as ready for review October 13, 2023 09:35
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@TonyXiang8787
Copy link
Member

@mgovers this PR needs more attention. Do not blindly change every indptr to Sparse mapping. Some of them should actually be dense mapping. This involves the physical knowledge to see what makes sense for a typical power grid.

@mgovers
Copy link
Member Author

mgovers commented Oct 14, 2023

@mgovers this PR needs more attention. Do not blindly change every indptr to Sparse mapping. Some of them should actually be dense mapping. This involves the physical knowledge to see what makes sense for a typical power grid.

Yes i agree. But i need your (or someone else's) input for that. Hence also my comment on the common data type.

  • I made sure to only ever use auto, so changing the classes is almost trivial. The from_sparse tag ensures that no constructor input needs to be changed for that change.
  • Changing the input data type is a littlebit more tricky, because it requires modifying e.g test data. But i strongly feel that this PR should only change classes, not test data, to separate dangerous refactorings (it's easy to make a mistake)

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
Copy link
Member Author

mgovers commented Oct 16, 2023

@mgovers this PR needs more attention. Do not blindly change every indptr to Sparse mapping. Some of them should actually be dense mapping. This involves the physical knowledge to see what makes sense for a typical power grid.

Yes i agree. But i need your (or someone else's) input for that. Hence also my comment on the common data type.

* I made sure to only ever use `auto`, so changing the classes is almost trivial. The `from_sparse` tag ensures that no constructor input needs to be changed for that change.

* Changing the input data type is a little bit more tricky, because it requires modifying e.g test data. But i strongly feel that this PR should only change classes, not test data, to separate dangerous refactorings (it's easy to make a mistake)

We agreed during standup that this is actually how we should approach this PR to prevent too many changes from happening in one go

@sonarcloud
Copy link

sonarcloud bot commented Oct 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

100.0% 100.0% Coverage
2.0% 2.0% Duplication

@mgovers mgovers merged commit 4754aa9 into main Oct 16, 2023
28 checks passed
@mgovers mgovers deleted the feature/refactor-replace-indptrs branch October 16, 2023 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement on internal implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants