-
Notifications
You must be signed in to change notification settings - Fork 6
Feat: Create PGM-DS Grid object from PGM dict #39
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
Feat: Create PGM-DS Grid object from PGM dict #39
Conversation
…. Signed-off-by: Marnix van Lieshout <[email protected]>
Signed-off-by: Marnix van Lieshout <[email protected]>
…lliander.com> I, Marnix van Lieshout <[email protected]>, hereby add my Signed-off-by to this commit: ac2b230 Signed-off-by: Marnix van Lieshout <[email protected]>
jaapschoutenalliander
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for setting up the PR, it is a good set up in my opinion.
On your questions,
- For me making the grid object optional is fine
- Using the default values is the only option I can see for creating a full array from partial information in the pgm-input
My main concern for this solution is the user provided array_mapping, I left a suggestion on that
Signed-off-by: Marnix van Lieshout <[email protected]>
|
Thanks for the feedback @jaapschoutenalliander, I will work on making some changes to the code over de next days. |
Signed-off-by: Marnix van Lieshout <[email protected]>
Signed-off-by: Marnix van Lieshout <[email protected]>
jaapschoutenalliander
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I have made some suggestions after which I think it is complete, mainly in test syntax
Signed-off-by: Marnix van Lieshout <[email protected]>
jaapschoutenalliander
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding the feature, we appreciate the effort!
Before merging I have one last request, to update the VERSION file to 1.2 https://github.com/PowerGridModel/power-grid-model-ds/blob/main/VERSION (also for your own ease of implementation)
Signed-off-by: Marnix van Lieshout <[email protected]>
Signed-off-by: Marnix van Lieshout [email protected]
Fixes issue: #27
Changes proposed in this PR include: