Use ports for PhasorDynamics signal connectivity

[lukelowry]

Description

This PR updates PhasorDynamics component wiring so signal nodes are associated with input/output ports instead of internal/external variable enums.

Closes #407

@PhilipFackler, what are your thoughts? Managed to do it with no *.case.json changes, which lets us do the INPUT_FORMAT changes and documentation with a few changes in code once we get to it

Proposed changes

• Split component connection data into terminals, input ports, and output ports.
• Keep the existing flat JSON "ports" format, but parse entries into the new internal maps.
• Update ComponentSignals, SystemModel, components, examples, and tests to use port enums for signal wiring.
• Remove the old variable-enum signal helpers.

Checklist

• All tests pass for the focused coverage.
• 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.
• I have updated CHANGELOG.md to reflect the changes in this PR.

Further comments

I organized the commits so we can roll back if the scope is too big, but it was mechanical and straightforward after the main changes were in.

[superwhiskers]

i'd recommend we make this one change. everything else seems fine. i have not attempted to compile or test the diff.

[superwhiskers]

i think we should keep this concept. above, in the json parsing code, you rely upon the existence of a SIZE variant in the enums. just swap the T::MAXIMUM requirement for T::SIZE.

additionally, there's many cases in this diff where you don't have any ports, and just leave an empty enum for one of the input/output/terminal port enums. it may be beneficial to keep the NoVariables enum---perhaps renamed---for those cases. that's less important, though.

[lukelowry]

Thank you for the feedback. Regarding your second comment about NoVariables: My rational was that some of these models can be implemented with or without internal variables (e.g. Branch and LoadZIP). I agree that having the model internal variables enum just contain SIZE is a bit useless, but it makes adding variables/ports easier and makes the model implementation more copy-and-paste-able.

What are your thoughts?

[superwhiskers]

i'm of the opinion that boilerplate exists to be minimized, so i don't view a copy-and-pasteable implementation to be a good thing. however, i can see how it could be useful for people unfamiliar with the codebase or c++ generally to be able to easily see how to add internal variables in their own model without cross-referencing more.

the former opinion is what leads me to suggest retaining a NoVariables-like placeholder. if you disagree with this stance, and want to make it simpler to copy-and-paste an implementation as a template wholesale, then go ahead with that. this was a minor concern anyway.

[pelesh]

As far as the naming conventions go, I would suggest using either "port" or "terminal" for the connectors but not a mixture of the two. The way I see it, we have three types of terminals (ports): bus, signal out and signal in.

I also suggest when making a wholesale name changes to have a purely name changing pull request to separate it from any design changes in the code. This makes reviewing PRs and tracking back changes easier.

[PhilipFackler]

As far as the naming conventions go, I would suggest using either "port" or "terminal" for the connectors but not a mixture of the two. The way I see it, we have three types of terminals (ports): bus, signal out and signal in.

Maybe this should be a question for users? I don't know power systems, but we do treat the bus connection differently. If it is conceptually different from signal connections, then it would make sense (to me) to call it something else to distinguish the two. But at the end of the day, what makes the most intuitive sense for users is what matters in this case.

[lukelowry]

@pelesh @PhilipFackler This branch has been updated to only have the naming changes, now following the pattern:

• *SignalInputs
• *SignalOutputs
• *Buses

Ready for review. mostly mechanical change

[PhilipFackler]

@lukelowry Just double-checking. I thought from our meeting, it was decided to use the word ports for all of them:

• *SignalInputPorts
• *SignalOutputPorts
• *BusPorts

It appears we did not write it down, though. :)

@pelesh do you remember?

[pelesh]

@lukelowry Just double-checking. I thought from our meeting, it was decided to use the word ports for all of them:

• *SignalInputPorts
• *SignalOutputPorts
• *BusPorts

It appears we did not write it down, though. :)

@pelesh do you remember?

We agreed to have

• *SignalInputs
• *SignalOutputs
• *Buses

and if there is a common name (i.e. base class) needed that would be *Ports.

[PhilipFackler]

Alright, sorry about my confusion.

[pelesh]

By looking at the implementation, the three are very different, so it doesn't seem we need Ports object in the code, but in the documentation we could refer to them as ports for brevity.

[PhilipFackler]

Looks good. Tested locally.