-
Notifications
You must be signed in to change notification settings - Fork 43
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
CGMES metadata models extension #2898
Conversation
Signed-off-by: Luma <zamarrenolm@aia.es>
Signed-off-by: Luma <zamarrenolm@aia.es>
After discussing with @annetill, the backward compatibility for extensions CgmesSshMetadata and CgmesSvMetedata does not need to be addressed. It seems that there's no usecase which needed to keep iidm files with those extensions (see slack question in the #powsybl channel). You can thus delete these extensions and their SerDe. If a (strong) need is expressed later, we'll deal with it with a corrective release. |
cgmes/cgmes-extensions/src/main/java/com/powsybl/cgmes/extensions/CgmesMetadataModelsSerDe.java
Outdated
Show resolved
Hide resolved
…sion for storing metadata Signed-off-by: Luma <zamarrenolm@aia.es>
Signed-off-by: Luma <zamarrenolm@aia.es>
Signed-off-by: Luma <zamarrenolm@aia.es>
Signed-off-by: Luma <zamarrenolm@aia.es>
Signed-off-by: Luma <zamarrenolm@aia.es>
To support the use-case of CGM export we need to be able to build an SV model with "dependent on" references to its IGM updated SSH models and also to the original IGM TP models. In a CGM IIDM When a user wants to make a CGM export, first he has to export the SSH of each To keep the required information in the extension (the data about the exported SSH), we plan to update the extension every time an export is made. To avoid the extension growing indefinitely, we will only keep the last model for each part and modelling authority. |
Signed-off-by: Luma <zamarrenolm@aia.es>
Signed-off-by: Luma <zamarrenolm@aia.es>
Signed-off-by: Luma <zamarrenolm@aia.es>
After an internal discussion we prefer not to use the metadata extension to store results of export. Instead, to support the use case of CGM SSH, SV export, we will use an export context. |
Signed-off-by: Luma <zamarrenolm@aia.es>
Signed-off-by: Luma <zamarrenolm@aia.es>
@@ -257,6 +257,11 @@ public PropertyBags fullModel(String cgmesProfile) { | |||
return namedQuery("fullModel", cgmesProfile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it has been removed in the last commit
Signed-off-by: Luma <zamarrenolm@aia.es>
public PropertyBags fullModel(String cgmesProfile) { | ||
return new PropertyBags(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you not implement the fullModels
method here? I am not sure how these classes are used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was used to obtain the CGMES metadata model for a specific part. Now we want to make a single query to the CGMES database to obtain all the metadata models. We do not need anymore the method fullModel(String cgmesProfile)
. We have replaced it by fullModels()
.
@@ -28,6 +28,37 @@ SELECT * | |||
OPTIONAL { ?FullModel md:Model.description ?description } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the fullModel
query should be removed too no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is not used anymore. I have removed it.
@@ -0,0 +1,128 @@ | |||
/** | |||
* Copyright (c) 2022, RTE (http://www.rte-france.com) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it not be 2024? The identifier is missing too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, fixed.
Signed-off-by: Luma <zamarrenolm@aia.es>
Signed-off-by: Luma <zamarrenolm@aia.es>
<xs:element name="cgmesMetadataModels"> | ||
<xs:complexType> | ||
<xs:sequence> | ||
<xs:element name="model" type="cmm:Model" maxOccurs="unbounded"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a unbounded list of elements with a part
enum, it would be clearer to restrict to 4 optional named elements (SV / SSH / TP / EQ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want the extension to be generic enough to store metadata for other parts. The complete list is defined in CgmesSubset
. What do you think about defining the part as a simple enum type with the proper values? (like the Side in the branch observability IIDM extension)
EQUIPMENT
TOPOLOGY
STATE_VARIABLES
STEADY_STATE_HYPOTHESIS
DYNAMIC
DIAGRAM_LAYOUT
GEOGRAPHICAL_LOCATION
EQUIPMENT_BOUNDARY
TOPOLOGY_BOUNDARY
UNKNOWN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are storing in the extension only the information about the models that have been read during the import of the network. We could limit it even more, and store only some of the models we have read.
I think that having a limited list of named elements instead of a property would make future modifications more difficult. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I wasn't aware there are more! Ok to keep the list with a part
enum then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And about defining the part
as a simple enum type in the xsd, it's a good idea yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the part is now an enum in the xsd
Signed-off-by: Luma <zamarrenolm@aia.es>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the other comment, could you also add some javadoc? At least to the public methods that have been modified
private final CgmesMetadataModel exportedEQModel = new CgmesMetadataModel(CgmesSubset.EQUIPMENT, DEFAULT_MODELING_AUTHORITY_SET_VALUE); | ||
private final CgmesMetadataModel exportedTPModel = new CgmesMetadataModel(CgmesSubset.TOPOLOGY, DEFAULT_MODELING_AUTHORITY_SET_VALUE); | ||
private final CgmesMetadataModel exportedSVModel = new CgmesMetadataModel(CgmesSubset.STATE_VARIABLES, DEFAULT_MODELING_AUTHORITY_SET_VALUE); | ||
private final CgmesMetadataModel exportedSSHModel = new CgmesMetadataModel(CgmesSubset.STEADY_STATE_HYPOTHESIS, DEFAULT_MODELING_AUTHORITY_SET_VALUE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep those models hardcoded here? If another model is added, we'll have to add it here, add the corresponding methods (like getExportedEQModel()
), etc.
Would it not be better to store them for example in a map, using the CgmesSubset
enum as a key? It would allow to have only a getExporterModel(CgmesSubset cmgesSubset)
method instead of one for each model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the "models" attributes will be removed in #2927. I don't think that is necessary to refactor that part now.
Signed-off-by: Luma <zamarrenolm@aia.es>
Signed-off-by: Romain Courtier <romain.courtier@rte-france.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice, thanks all!
Signed-off-by: Romain Courtier <romain.courtier@rte-france.com>
|
My comments will be covered by a future PR |
Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
What kind of change does this PR introduce?
Feature + bug because of dependencies management during export.
What is the current behavior?
Multiple extensions to keep CGMES metadata:
What is the new behavior (if this is a feature change)?
CGMES metadata is kept in a single IIDM Network extension.
Does this PR introduce a breaking change or deprecate an API?
If yes, please check if the following requirements are fulfilled
What changes might users need to make in their application due to this PR? (migration steps)
Other information:
Bring to current development branch the initial work made in #2034