[FEATURE] Power Grid Meta Data attributes enum#1324
[FEATURE] Power Grid Meta Data attributes enum#1324furqan463 wants to merge 37 commits intoPowerGridModel:mainfrom
Conversation
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>
mgovers
left a comment
There was a problem hiding this comment.
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.
code_generation/templates/src/power_grid_model/_core/dataset_class_maps.py.jinja
Outdated
Show resolved
Hide resolved
code_generation/templates/src/power_grid_model/_core/dataset_class_maps.py.jinja
Outdated
Show resolved
Hide resolved
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
|
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 |
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>
|
@mgovers @nitbharambe I removed the changes in tests, |
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
furqan463
left a comment
There was a problem hiding this comment.
Updated examples being affected by changes.
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
|
@furqan463 Sorry, I've been a bit packed. I'll review as soon as possible. |
code_generation/templates/src/power_grid_model/_core/dataset_class_maps.py.jinja
Show resolved
Hide resolved
code_generation/templates/src/power_grid_model/_core/dataset_class_maps.py.jinja
Show resolved
Hide resolved
|
Minor refactoring etc related comments. Everything else looks good. |
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
Signed-off-by: furqan463 <ahmadfurqanc@gmail.com>
|
Manually copied the |
|
Manually copied the |
Handy tip: You can do |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hello @furqan463,
I just finished the review and left mostly minor remarks. Here are some additional ones:
- Can you also migrate to
AttributeTypein all the examples and then re-run the notebooks? That should be the "final" step of the migration. - I see that in the docs, there are other places where
str" valued attributes can be migrated toAttributeType, such as innative-data-interface.md,data-validator.md`, etc. Can you migrate those as well? - Some docstrings, for example in
data_types.pyhave not been updated toAttributeType. Can you double check and update them? - 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.
code_generation/templates/src/power_grid_model/_core/dataset_class_maps.py.jinja
Outdated
Show resolved
Hide resolved
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>
I saved the branch with all examples and tests using |
Updated |
i agree with all you said |
Fixes #949
Changes proposed in this PR include:
Created
AttributeTypeEnum containing attributes of all PGM components through code_generation. Implemented use of new Enum with backward compatibility.Checks