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 C API #711

Merged
merged 23 commits into from
Sep 17, 2024
Merged

Feature / Columnar data C API #711

merged 23 commits into from
Sep 17, 2024

Conversation

Jerry-Jinfeng-Guo
Copy link
Contributor

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo commented Sep 3, 2024 •
edited
Loading

Implements C API changes in #548

  • interface
  • add to c++ api wraper
  • c api tests
    • api model
    • serialization
  • fix ... things that inevitably need fixing

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo added the feature New feature or request label Sep 3, 2024
@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo self-assigned this Sep 3, 2024
@Jerry-Jinfeng-Guo
added for const and few minor fixes
1b763bc 
Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
clang-tidy
db9189e 
Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
[skip ci] initial test added to add attribute buffer to the input dat…
0c3e730 
…aset. `main_model_impl` still lacks functionality to accommodate columnar data input

Signed-off-by: Jerry Guo <[email protected]>
@mgovers mgovers changed the base branch from main to feature/main-model-columnar-data September 9, 2024 14:31
@mgovers mgovers force-pushed the feature/columnar-data-c-api branch from 2001844 to 874c770 Compare September 10, 2024 07:09
@mgovers
clang-tidy
354dbec 
Signed-off-by: Martijn Govers <[email protected]>
mgovers
mgovers reviewed Sep 10, 2024
View reviewed changes
Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

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

great cleanup on the clang-tidy

tests/c_api_tests/test_cpp_api_model.cpp Outdated Show resolved Hide resolved
@mgovers
minor fixes
ab9a41d 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers
fail to fixme
405fd94 
Signed-off-by: Martijn Govers <[email protected]>
Base automatically changed from feature/main-model-columnar-data to main September 10, 2024 09:59
An error occurred while trying to automatically change base from feature/main-model-columnar-data to main September 10, 2024 09:59
@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo marked this pull request as ready for review September 13, 2024 14:44
figueroa1395
figueroa1395 reviewed Sep 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.

Looks good to me overall, just minor comments and the possible addition of tests for add_attribute_buffer for deserialized data, may be needed.

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
power_grid_model_c/power_grid_model_cpp/include/power_grid_model_cpp/handle.hpp Show resolved Hide resolved
tests/c_api_tests/test_cpp_api_model.cpp Outdated Show resolved Hide resolved
tests/c_api_tests/test_cpp_api_model.cpp Outdated Show resolved Hide resolved
tests/c_api_tests/test_cpp_api_serialization.cpp Outdated Show resolved Hide resolved
tests/c_api_tests/test_cpp_api_serialization.cpp Outdated Show resolved Hide resolved
TonyXiang8787
TonyXiang8787 requested changes Sep 16, 2024
View reviewed changes
Copy link
Member

@TonyXiang8787 TonyXiang8787 left a comment

Choose a reason for hiding this comment

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

@Jerry-Jinfeng-Guo please resolve the merge conflict and we should be good to go

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo marked this pull request as draft September 16, 2024 11:43
@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo marked this pull request as ready for review September 16, 2024 15:40
@TonyXiang8787
Copy link
Member

Hmmm. buffer overflow...

@Jerry-Jinfeng-Guo
bug fixed (gcc)
bc21719 
Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
Copy link
Contributor Author

Hmmm. buffer overflow...

Fixed... |complete json data| < |json data|

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
11 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@TonyXiang8787 TonyXiang8787 added this pull request to the merge queue Sep 17, 2024
Merged via the queue into main with commit 5e199a4 Sep 17, 2024
26 checks passed
@TonyXiang8787 TonyXiang8787 deleted the feature/columnar-data-c-api branch September 17, 2024 07:27
@mgovers mgovers mentioned this pull request Nov 5, 2024
Closed
27 tasks
@figueroa1395 figueroa1395 mentioned this pull request Nov 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@figueroa1395 figueroa1395 figueroa1395 left review comments

@mgovers mgovers mgovers left review comments

@TonyXiang8787 TonyXiang8787 TonyXiang8787 approved these changes

Assignees

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo

Labels
feature New feature or request
Projects
Power Grid Model
Archived in project
Milestone
No milestone
Development

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