Skip to content

improve the approach we use for parsing and storing component data#169

Merged
pelesh merged 16 commits intodevelopfrom
improve-component-data-interface
Jul 9, 2025
Merged

improve the approach we use for parsing and storing component data#169
pelesh merged 16 commits intodevelopfrom
improve-component-data-interface

Conversation

@superwhiskers
Copy link
Copy Markdown
Collaborator

@superwhiskers superwhiskers commented Jul 2, 2025

this pull request replaces the data structures for components introduced in #133, save for those related to buses. this is done to simplify the way json parsing is handled for them

checklist

  • All tests pass.
  • Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
  • The new code follows GridKit™ style guidelines.
  • There are unit tests for the new code.
  • The new code is documented.
  • The feature branch is rebased with respect to the target branch.

@superwhiskers superwhiskers requested a review from pelesh July 2, 2025 23:08
Copy link
Copy Markdown
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

Class BusFault.cpp includes (indirectly) both, nlohmann/json.hpp and magic_enum/magic_enum.hpp headers. This makes the component dependent on I/O file parser implementation. We need to find a way to abstract away component models from the parser specifics. All component models should build independently of the parser (where it is okay if the parser depends on components).

@superwhiskers
Copy link
Copy Markdown
Collaborator Author

i'm not sure it's really possible to decouple it any further unless we added even more intermediary structures or something. both of those headers could be removed and with some conditional compilation, since all of the actual json parsing code resides in the one file, it'd be easy to remove the json parser and magic_enum. i don't think attempting to completely remove an indirect dependency is particularly worthwhile here.

@superwhiskers
Copy link
Copy Markdown
Collaborator Author

well, actually now that i recall there's one field in the ComponentData.hpp file that needs magic_enum. the json parser, however, can be removed completely. it technically doesn't even have to be in that same file (but i put it in there because it simplifies things for the user as they wouldn't have to include a separate "json parser" file to use the json parser with components) as the json parser we use only requires things to be 1) in the same namespace1 and 2) included within the file they are being used within

Footnotes

  1. technically it's not required but it makes things simpler

@pelesh
Copy link
Copy Markdown
Collaborator

pelesh commented Jul 3, 2025

Also parser tests should not be in phasor dynamics but in a separate subdirectory. They are helper utility, not the part of phasor dynamics core.

@pelesh
Copy link
Copy Markdown
Collaborator

pelesh commented Jul 3, 2025

i'm not sure it's really possible to decouple it any further unless we added even more intermediary structures or something.

Then I guess we should add intermediary data structures, i.e. meta models. Also, we should consider using factories to instantiate and parametrize components.

@superwhiskers
Copy link
Copy Markdown
Collaborator Author

Then I guess we should add intermediary data structures, i.e. meta models

we already have an intermediary (the *Data family of structures). i'm concerned adding even more would overcomplicate the entire set of classes and data structures we have and for little tangible benefit. i already think what we're doing here is a little much, but i can see the benefit (decoupling our internal representation from the exposed one). adding even more seems unnecessary and would duplicate representations all over the place.

we can already remove the json parser from the ComponentData structure easily and i don't see the benefit of going any further than that as the data structure itself has no reliance upon the existence of it and it's served its purpose (making individual *Data structures not need to deal with json parsing and simplifying the process of adding new components).

adding even more structures would be detrimental to your original goal, i think. it would require consumers of the library to interact with more structures to get things done (unless i'm interpreting the proposal wrong) and again, duplicate places in which data can be and incur a greater maintenance burden upon us. without more justification and a clearer path to implementation i'm against making this change, personally.

@pelesh pelesh requested review from abirchfield and lukelowry July 3, 2025 18:01
@superwhiskers superwhiskers force-pushed the improve-component-data-interface branch from 838c09d to f2df29b Compare July 8, 2025 15:19
@superwhiskers superwhiskers requested a review from pelesh July 8, 2025 15:20
@superwhiskers superwhiskers marked this pull request as ready for review July 8, 2025 15:20
@superwhiskers
Copy link
Copy Markdown
Collaborator Author

this should be ready to go now

@superwhiskers superwhiskers force-pushed the improve-component-data-interface branch from 84fe668 to 4a4d72c Compare July 9, 2025 12:06
@superwhiskers
Copy link
Copy Markdown
Collaborator Author

rebased on latest develop and made changes to fix the new ThreeBusClassical test

Copy link
Copy Markdown
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

Just a couple of nitpicking comments.

Comment thread CMakeLists.txt Outdated
Comment thread src/Model/PhasorDynamics/Load/LoadData.hpp Outdated
@superwhiskers superwhiskers requested a review from pelesh July 9, 2025 16:08
@pelesh pelesh merged commit ee5c7bf into develop Jul 9, 2025
@superwhiskers superwhiskers deleted the improve-component-data-interface branch July 9, 2025 17:13
WiktoriaZielinskaORNL pushed a commit that referenced this pull request Jul 23, 2025
)

* move the case format tests and require floating point parameters to be floating point literals again. no implicit conversions

* add some documentation to the modified bus fault data structure

* break out the component data json parser into a separate file

* finish reworking the remainder of the applicable `*Data` structures to use `ComponentData`

* fix documentation comment in `LoadImpl.hpp`

* remove extraneous parens in `SystemModelData.hpp`

* fix the `ThreeBusClassical` example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants