Skip to content

[FEATURE] Power Grid Meta Data attributes enum#1324

Open
furqan463 wants to merge 37 commits intoPowerGridModel:mainfrom
furqan463:attribute_enums
Open

[FEATURE] Power Grid Meta Data attributes enum#1324
furqan463 wants to merge 37 commits intoPowerGridModel:mainfrom
furqan463:attribute_enums

Conversation

@furqan463
Copy link
Contributor

@furqan463 furqan463 commented Mar 8, 2026

Fixes #949

Changes proposed in this PR include:

Created AttributeType Enum containing attributes of all PGM components through code_generation. Implemented use of new Enum with backward compatibility.

Checks

  • Make sure there's no breaking change.
  • Review and Update docs

furqan463 added 11 commits March 7, 2026 23:19
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
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.

Very nice improvement. However, I foresee some breaking changes in the current implementation. Either we need some type deduction overloads or we need to keep some names similar. It's unfortunate but it's the way it is.

Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
@figueroa1395
Copy link
Member

Hello @furqan463,

We had some additional discussion, and since this is a very involving PR that touches API, we thought that I'd be good if we had a call with you to smooth the process and help where needed more easily. That is of course if you would like to as well. I have contacted you via email to privately discuss this.

In addition, we have an additional remark: the handling of dtype for the attributes; something that wasn't relevant before for the dataset and component types. We didn't dig into it yet, but given that you are very ahead in this PR, could you take a look at it? If it get's too complicated, we can scope it out, but mypy should be happy about it nevertheless.

Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
@furqan463
Copy link
Contributor Author

@mgovers @nitbharambe I removed the changes in tests,
However, I'm made a few changes in tests to fix typing errors assuming validation rules and validation errors are for internal use and not an interface.
If otherwise, please let me know.

Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Copy link
Contributor Author

@furqan463 furqan463 left a comment

Choose a reason for hiding this comment

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

Updated examples being affected by changes.

Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
@figueroa1395
Copy link
Member

@furqan463 Sorry, I've been a bit packed. I'll review as soon as possible.

@nitbharambe
Copy link
Member

Minor refactoring etc related comments. Everything else looks good.

Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
@furqan463 furqan463 requested a review from nitbharambe March 12, 2026 17:20
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
@furqan463
Copy link
Contributor Author

Manually copied the src folder to pgm installation in pgm-io environment and ran all tests (unit & validation), all tests are successful.

@furqan463
Copy link
Contributor Author

Manually copied the src folder to pgm installation in pgm-ds environment and ran all tests, all tests are successful.

@nitbharambe
Copy link
Member

Manually copied the src folder to pgm installation in pgm-io environment and ran all tests (unit & validation), all tests are successful.

Handy tip: You can do uv pip install -e path_of_dependent_repository (or only pip install -e) to do an editable install of that package. Hence you dont have to copy things each time there is an update in the dependent repository. (https://pydevtools.com/handbook/explanation/what-is-an-editable-install/)

@nitbharambe nitbharambe added the feature New feature or request label Mar 13, 2026
@nitbharambe nitbharambe added the do-not-merge This should not be merged label Mar 16, 2026
nitbharambe
nitbharambe previously approved these changes Mar 16, 2026
Copy link
Member

@nitbharambe nitbharambe left a comment

Choose a reason for hiding this comment

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

Since @figueroa1395 has dived deeper into this before, let him have a final look too. (Also a 2nd look since its an API change). Added do-not-merge for that reason.

Copy link
Member

@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.

Hello @furqan463,

I just finished the review and left mostly minor remarks. Here are some additional ones:

  1. Can you also migrate to AttributeType in all the examples and then re-run the notebooks? That should be the "final" step of the migration.
  2. I see that in the docs, there are other places where str" valued attributes can be migrated to AttributeType, such as in native-data-interface.md, data-validator.md`, etc. Can you migrate those as well?
  3. Some docstrings, for example in data_types.py have not been updated to AttributeType. Can you double check and update them?
  4. Some instances of attributes, for example in data_types.py (quick search for use of "id" shows it) are still using 'str' version in-code. Can you replace them.

For 1. and 2. I would recommend to do on a follow up documentation PR to prevent this one for growing even more as it becomes very hard to review.

For 3. and 4. are needed to complete the PR, but if we accidentally miss some instances that's fine (we can catch them later)... thinking about it, maybe this can also be done in a follow up, since this PR contains all the logic changes, any opinion on this @furqan463?

@nitbharambe @mgovers What do you think about the above suggestions? It'd make it easier but I'm also hesitant because this PR touches API and such.

Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
@furqan463
Copy link
Contributor Author

For 1. and 2. I would recommend to do on a follow up documentation PR to prevent this one for growing even more as it becomes very hard to review.

I saved the branch with all examples and tests using AttributeType instead of strings for a followup PR.

@furqan463
Copy link
Contributor Author

For 3. and 4. are needed to complete the PR, but if we accidentally miss some instances that's fine

Updated data_types.py.

@mgovers
Copy link
Member

mgovers commented Mar 17, 2026

Hello @furqan463,

I just finished the review and left mostly minor remarks. Here are some additional ones:

1. Can you also migrate to `AttributeType` in all the examples and then re-run the notebooks? That should be the "final" step of the migration.

2. I see that in the docs, there are other places where `str" valued attributes can be migrated to `AttributeType`, such as in `native-data-interface.md`, `data-validator.md`, etc. Can you migrate those as well?

3. Some docstrings, for example in `data_types.py` have not been updated to `AttributeType`. Can you double check and update them?

4. Some instances of attributes, for example in `data_types.py` (quick search for use of "id" shows it) are still using 'str' version in-code. Can you replace them.

For 1. and 2. I would recommend to do on a follow up documentation PR to prevent this one for growing even more as it becomes very hard to review.

For 3. and 4. are needed to complete the PR, but if we accidentally miss some instances that's fine (we can catch them later)... thinking about it, maybe this can also be done in a follow up, since this PR contains all the logic changes, any opinion on this @furqan463?

@nitbharambe @mgovers What do you think about the above suggestions? It'd make it easier but I'm also hesitant because this PR touches API and such.

i agree with all you said

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge This should not be merged feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Power Grid Meta Data attributes enum

4 participants