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/remove legacy dataset #587

Merged
merged 76 commits into from
May 31, 2024
Merged

Feature/remove legacy dataset #587

merged 76 commits into from
May 31, 2024

Conversation

TonyXiang8787
Copy link
Member

@TonyXiang8787 TonyXiang8787 commented May 6, 2024 •
edited

Closes #431

Also make meta_data as input argument for many classes, instead of hard code to the automatically generated constexpr version. In this way, it is possible to do unit test with mocked meta_data.

Check list

  • Calculation core
  • C API
  • C Example
  • C API test
  • Unit test
  • Integration test
  • Validation test
  • Benchmark

@TonyXiang8787
start dataset
0128e9e 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
refactor dataset
c048b8c 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
refactor dataset
abe16dd 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
finish dataset
6eb813a 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
try serialization
60c6795 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
only model left
0d2cf39 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
start functor
6a5d26e 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
build and update
0c0ecf2 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
modify
a186b42 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
main core
60bcc36 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
get index seq
80e73be 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
start batch
1b6b9cb 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
seek check independent
4f61a6c 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
solve batch size
fdfdf9e 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
independent
6ec9792 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
independent
5ee11b5 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
c api not working yet
296a9f7 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
check compiler
3a29754 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
compile pass
d94761f 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
example pass
457afdd 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
issue with dataset
2ffff45 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
think about decouple meta data
675d043 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
try to add meta data
334effb 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
almost done
164cb5d 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
c api and example
51b6f73 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
c api test
84d49ea 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
try optimizer
8c012ee 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787 TonyXiang8787 requested a review from mgovers May 6, 2024 13:41
@TonyXiang8787
Copy link
Member Author

@mgovers It is far from finished. I just put here to see what you think. The tap position optimizer, we still need to make it compile with mocked meta_data.

@mgovers
Copy link
Member

mgovers commented May 6, 2024

@mgovers It is far from finished. I just put here to see what you think. The tap position optimizer, we still need to make it compile with mocked meta_data.

i'll have a look

@TonyXiang8787 TonyXiang8787 marked this pull request as ready for review May 28, 2024 12:08
@TonyXiang8787
Copy link
Member Author

@mgovers it's now ready for review

@TonyXiang8787
clang-format-15
d994df8 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
clang-tidy
a8f7c13 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
initialization
0c4f34d 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
force indptr to nullptr
e211f57 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@mgovers
Copy link
Member

mgovers commented May 29, 2024

@mgovers it's now ready for review

will have a look

Quality Gate Passed Quality Gate passed

Issues 18 New issues 0 Accepted issues

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

See analysis details on SonarCloud

can you please have a look at the address sanitizer error in the gcc release build + the sonar cloud issues?

@TonyXiang8787
fix gcc release bug
def90c6 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
Copy link
Member Author

@mgovers it's now ready for review

will have a look

Quality Gate Passed Quality Gate passed

Issues 18 New issues 0 Accepted issues
Measures 0 Security Hotspots 93.5% Coverage on New Code 0.0% Duplication on New Code
See analysis details on SonarCloud

can you please have a look at the address sanitizer error in the gcc release build + the sonar cloud issues?

gcc build is fixed.

@TonyXiang8787
sonarcloud
1e6468d 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
revert emplace back
ad47d98 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
mgovers
mgovers reviewed May 29, 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.

I'll continue tomorrow

tests/benchmark_cpp/fictional_grid_generator.hpp Outdated Show resolved Hide resolved
tests/benchmark_cpp/fictional_grid_generator.hpp Outdated 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/auxiliary/dataset.hpp Outdated Show resolved Hide resolved
tests/cpp_integration_tests/test_main_model.cpp Outdated 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/auxiliary/dataset.hpp Outdated Show resolved Hide resolved
tests/cpp_integration_tests/test_main_model_se.cpp Show resolved Hide resolved
tests/cpp_integration_tests/test_main_model_static.cpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model.hpp Show resolved Hide resolved
mgovers
mgovers reviewed May 30, 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.

i think i've gone through the entire PR. great improvement

power_grid_model_c/power_grid_model/include/power_grid_model/main_model.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model.hpp 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 Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/auxiliary/dataset.hpp Outdated Show resolved Hide resolved
@TonyXiang8787
Merge remote-tracking branch 'origin/main' into feature/remove-legacy…
2334856 
…-dataset
@TonyXiang8787
[skip ci] use integral concept
57030a1 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
[skip ci] integral concept
3a4312e 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
[skip ci] integral concept
5987ad8 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
[skip ci] ranges
f400145 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@TonyXiang8787
Copy link
Member Author

@mgovers all comments resolved.

@TonyXiang8787
formatting
98cab37 
Signed-off-by: Tony Xiang <tony.xiang@alliander.com>
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 31, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
6 Accepted issues

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

See analysis details on SonarCloud

@mgovers
Copy link
Member

mgovers commented May 31, 2024

out of scope of this ticket but we may need to version bump sonar:

image

@TonyXiang8787
Copy link
Member Author

out of scope of this ticket but we may need to version bump sonar:

image

Do we just have to modify like this?
https://docs.sonarsource.com/sonarcloud/advanced-setup/languages/c-c-objective-c/#executing-the-analysis

@mgovers
Copy link
Member

mgovers commented May 31, 2024

out of scope of this ticket but we may need to version bump sonar:
image

Do we just have to modify like this? https://docs.sonarsource.com/sonarcloud/advanced-setup/languages/c-c-objective-c/#executing-the-analysis

i think so, yes. i haven't had a look at what the compile_commands.json are supposed to be in our particular case, but that's indeed what it complains about

mgovers
mgovers approved these changes May 31, 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.

LGTM, no more remarks

@TonyXiang8787 TonyXiang8787 added this pull request to the merge queue May 31, 2024
Merged via the queue into main with commit bc12764 May 31, 2024
26 checks passed
@TonyXiang8787 TonyXiang8787 deleted the feature/remove-legacy-dataset branch May 31, 2024 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgovers mgovers mgovers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove the Dataset logic from PGM core, use DatasetHandler for MainModel
2 participants
@TonyXiang8787 @mgovers