Skip to content
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

Add backward compatibility for CgmesControlAreas extension deserialization #3298

Open
wants to merge 6 commits into
base: remove_cgmes_control_areas_extension
Choose a base branch
from

Conversation

olperr1
Copy link
Member

@olperr1 olperr1 commented Jan 30, 2025

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Does this PR already have an issue describing the problem?

No

What kind of change does this PR introduce?

Feature

Does this PR introduce a new Powsybl Action implying to be implemented in simulators or pypowsybl?

  • Yes
  • No

What is the current behavior?

#3149 removes the CgmesControlAreas in favor of the usage of IIDM Areas, but the backward compatibility with the extension is not supported. Therefore when importing an old generated IIDM network containing the extension, the content of this extension is not loaded and no CGMES areas are created.

What is the new behavior (if this is a feature change)?

With this PR, the serialized extension is read and IIDM Areas are created for each CgmesControlArea of the extension.

Note that when reading a CgmesControlArea from the serialized extension, if an Area with the same ID is already present on the network, the CgmesControlArea is skipped.

Does this PR introduce a breaking change or deprecate an API?

  • Yes
  • No

If yes, please check if the following requirements are fulfilled

  • The Breaking Change or Deprecated label has been added
  • The migration steps are described in the following section

What changes might users need to make in their application due to this PR? (migration steps)

Other information:

For now, only the reading compatibility is supported, because it is not trivial to implement the writing compatibility: there's no extension corresponding to the CGMES areas on the network, so the extension serialization mechanism could not be used directly

@olperr1 olperr1 marked this pull request as ready for review January 30, 2025 15:56
@olperr1 olperr1 self-assigned this Jan 30, 2025
@zamarrenolm
Copy link
Member

I have tested the backward compatibility using a real world test case and it works perfectly.

Signed-off-by: Olivier Perrin <[email protected]>

@Override
public void write(DummyExt extension, SerializerContext context) {
throw new UnsupportedOperationException("Backward writing compatibility is not yet supported.");
Copy link
Contributor

Choose a reason for hiding this comment

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

yet means we'll do it one day, will we? Besides shouldn't this rather be something like "cannot happen"? We're here with a DummyExt which is never created, and cannot be created.

* @author Olivier Perrin {@literal <olivier.perrin at rte-france.com>}
*/
@AutoService(ExtensionSerDe.class)
public class CgmesControlAreasSerDe extends AbstractVersionableNetworkExtensionSerDe<Network, CgmesControlAreasSerDe.DummyExt> {
Copy link
Contributor

@flo-dup flo-dup Feb 11, 2025

Choose a reason for hiding this comment

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

why not using AbstractExtensionSerDe instead of AbstractVersionableNetworkExtensionSerDe? I don't get why we need the versionable part as there's only one the 1.0 version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes you're right, there's no need for the versionable management.
I will replace it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum no, sorry. I wanted to limit the extension to the IIDM versions <= 1.13.
The extension should not be accepted for the newer versions of IIDM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, there's no harm in accepting it for versions > 1.13, even though it will never occur, as there won't be any way to get a 1.14 xiidm with such an extension.

@olperr1 olperr1 requested a review from flo-dup February 11, 2025 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

4 participants