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.

Sign up for GitHub

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

Jump to bottom

Feature / columnar data optional id field support in update data #771

Merged
merged 141 commits into from
Nov 5, 2024
Merged

Feature / columnar data optional id field support in update data #771

merged 141 commits into from
Nov 5, 2024

Conversation

Jerry-Jinfeng-Guo
Copy link
Contributor

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo commented Oct 8, 2024 •
edited
Loading

This PR implements step 7 of #548

  • Validation on the combinations of different situations of id
  • Functionalities to generate sequential id for qualified input
  • Update columnar component with generated sequence id
  • Basic tests
    • Integration tests
    • API tests
    • Updated wrong id tests
    • Python tests
  • Extensive tests ??
    • Unit tests for new functionality
    • Validation for input without id

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo added the feature New feature or request label Oct 8, 2024
@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo self-assigned this Oct 8, 2024
Jerry-Jinfeng-Guo and others added 14 commits October 8, 2024 17:37
@Jerry-Jinfeng-Guo
Merge branch 'main' into feature/columnar-data-support-no-id-field
1edc403
@Jerry-Jinfeng-Guo
[skip ci]
8ded552 
Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
Merge branch 'main' into feature/columnar-data-support-no-id-field
ea6ba47
@Jerry-Jinfeng-Guo
[skip ci] added logic for has_id and id_nan
575e24d 
Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
[skip ci] more component level properties
0ea07fa 
Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
[skip ci] minor fix on the independence criteria
75b1583 
Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
[skip ci] added elements per scenario of base; subsequent comparison …
721eec4 
…made easier

Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
[skip ci] added checks
464ed64 
Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
[skip ci] functionality accepting sequence
d2ce49d 
Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
[skip ci] functionalities for get sequence in place; next to the upda…
3280fa8 
…te columnar component with sequence

Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
[skip ci] removing id match assert: incompatible with optional id; wh…
faa5966 
…ole pipeline through; one more test to update

Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
[skip ci] empty columnar data input supported
88610d2 
Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
[skip ci] made test more complete; next to merge with main
f584870 
Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
[skip ci] removed commented code
9a11e6a 
Signed-off-by: Jerry Guo <[email protected]>
mgovers
mgovers reviewed Oct 16, 2024
View reviewed changes
power_grid_model_c/power_grid_model/include/power_grid_model/component/load_gen.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/component/load_gen.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/component/power_sensor.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/component/voltage_sensor.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/component/voltage_sensor.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Show resolved Hide resolved
figueroa1395
figueroa1395 reviewed Oct 16, 2024
View reviewed changes
Copy link
Contributor

@figueroa1395 figueroa1395 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

My computer is forcing an update that requieres restart, I will continue the review later :)

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/update.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Outdated Show resolved Hide resolved
figueroa1395
figueroa1395 reviewed Oct 16, 2024
View reviewed changes
Copy link
Contributor

@figueroa1395 figueroa1395 left a comment

Choose a reason for hiding this comment

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

Finished the whole review.

power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Outdated Show resolved Hide resolved
tests/cpp_integration_tests/test_main_model.cpp Show resolved Hide resolved
@Jerry-Jinfeng-Guo
[skip ci] branch created
890837a 
Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo mentioned this pull request Oct 17, 2024
Merged
5 tasks
figueroa1395
figueroa1395 reviewed Nov 5, 2024
View reviewed changes
Copy link
Contributor

@figueroa1395 figueroa1395 left a comment

Choose a reason for hiding this comment

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

Minor comments, and some are left for the refactoring, so feel free to resolve some right away if needed. We are basically there! :D

power_grid_model_c/power_grid_model/include/power_grid_model/auxiliary/dataset.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/auxiliary/dataset.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/auxiliary/dataset.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Outdated Show resolved Hide resolved
tests/cpp_integration_tests/test_main_model.cpp Show resolved Hide resolved
tests/cpp_integration_tests/test_main_model.cpp Show resolved Hide resolved
tests/native_api_tests/test_api_model.cpp Outdated Show resolved Hide resolved
mgovers and others added 13 commits November 5, 2024 13:46
@mgovers
Apply suggestions from code review
0513ffa 
Signed-off-by: Martijn Govers <[email protected]>

Signed-off-by: Martijn Govers <[email protected]>
@mgovers
sonar-cloud
f098446 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers
minor
1faab9f 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers
Apply suggestions from code review
314af8a 
Signed-off-by: Martijn Govers <[email protected]>

Signed-off-by: Martijn Govers <[email protected]>
@mgovers
more resolve comments
1344c9f 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers
fix unused captured variable
d53d3ea 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers
fix variable name
02bf930 
Signed-off-by: Martijn Govers <[email protected]>
@Jerry-Jinfeng-Guo
Merge branch 'feature/columnar-data-support-no-id-field' into feature…
aa63dc8 
…/row-update-data-support-no-id-field

Signed-off-by: Jerry Guo <[email protected]>
@mgovers
fix typo
8762766 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers
Merge branch 'feature/columnar-data-support-no-id-field' into feature…
cfb4d1a 
…/row-update-data-support-no-id-field
@Jerry-Jinfeng-Guo
fix error introduced from merging
dba5644 
Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
Merge branch 'main' into feature/columnar-data-support-no-id-field
80d25eb 
Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
fix merge error
0a1ea5d 
Signed-off-by: Jerry Guo <[email protected]>
Jerry-Jinfeng-Guo and others added 5 commits November 5, 2024 15:07
@Jerry-Jinfeng-Guo
Merge branch 'feature/columnar-data-support-no-id-field' into feature…
69b736b 
…/row-update-data-support-no-id-field
@Jerry-Jinfeng-Guo
Merge pull request #813 from PowerGridModel/feature/row-update-data-s…
2c76f9c 
…upport-no-id-field

Feature / row based data optional id field support in update data
@mgovers
Revert "remove optional ids tests"
a6320cf 
This reverts commit e5d5670.

Signed-off-by: Martijn Govers <[email protected]>
@mgovers
remove pytest mark xfail
849d293 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers
format
74f2fef 
Signed-off-by: Martijn Govers <[email protected]>
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 5, 2024

Quality Gate Passed Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
94.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@mgovers
Copy link
Member

mgovers commented Nov 5, 2024

I think the remaining sonar cloud warnings can be considered out of scope of this PR

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo added this pull request to the merge queue Nov 5, 2024
Merged via the queue into main with commit 373d9e4 Nov 5, 2024
26 checks passed
@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo deleted the feature/columnar-data-support-no-id-field branch November 5, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TonyXiang8787 TonyXiang8787 TonyXiang8787 left review comments

@nitbharambe nitbharambe nitbharambe approved these changes

@figueroa1395 figueroa1395 figueroa1395 approved these changes

@mgovers mgovers mgovers approved these changes

Assignees

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo

Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Jerry-Jinfeng-Guo @mgovers @TonyXiang8787 @figueroa1395 @nitbharambe