-
Notifications
You must be signed in to change notification settings - Fork 10
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 storage extension #827
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Naledi EL CHEIKH <naledi.elcheikh@rte-france.com>
Signed-off-by: Naledi EL CHEIKH <naledi.elcheikh@rte-france.com>
Signed-off-by: Naledi EL CHEIKH <naledi.elcheikh@rte-france.com>
Signed-off-by: Naledi EL CHEIKH <naledi.elcheikh@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.
I think it would be useful to add a check in python, probably at least in the python test test_get_extensions_information to verify the mapping is correct
Signed-off-by: Naledi EL CHEIKH <naledi.elcheikh@rte-france.com>
|
||
private Stream<CgmesMetadataModel> itemsStream(Network network) { | ||
if (network.getExtension(CgmesMetadataModels.class) == null) { | ||
throw new PowsyblException("No CGMES metadata model found"); |
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.
To avoid throwing error if there is no extension, it would be better to just return no element :
throw new PowsyblException("No CGMES metadata model found"); | |
return Stream.empty() |
This way in the python we only get an empty dataframe
.strings("profiles", cgmesMetadataModel -> String.valueOf(cgmesMetadataModel.getProfiles())) | ||
.strings("dependent_on", cgmesMetadataModel -> String.valueOf(cgmesMetadataModel.getDependentOn())) | ||
.strings("supersedes", cgmesMetadataModel -> String.valueOf(cgmesMetadataModel.getSupersedes())) |
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.
If we decide to go with 'profile1/profile2/...', you will need to change these methods to create the concatenated string
public void removeExtensions(Network network, List<String> ids) { | ||
ids.stream().filter(Objects::nonNull) | ||
.map(id -> network.getExtension(CgmesMetadataModels.class)) | ||
.filter(Objects::nonNull) | ||
.forEach(model -> ((CgmesMetadataModels) model).getModels().remove(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.
It does not work for now (you try to remove multiple times the same extension).
To be consistent with other extensions, this method should remove the individual CgmesMetadataModel by selecting them from the list ids
SeriesUtils.applyIfPresent(profiles, row, modelAdder::addProfile); | ||
SeriesUtils.applyIfPresent(dependentOn, row, modelAdder::addDependentOn); | ||
SeriesUtils.applyIfPresent(supersedes, row, modelAdder::addSupersedes); |
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 methods modelAdder::add...
should be replaced with a method that
- takes the 'element1/element2/...' string as an input
- parse it as a list containing the different elements
- calls
modelAdder::add...
for every element (adding them to the corresponding set)
void create(int row, CgmesMetadataModelsAdder adder) { | ||
CgmesMetadataModelsAdder.ModelAdder modelAdder = adder.newModel(); | ||
SeriesUtils.applyIfPresent(id, row, modelAdder::setId); | ||
SeriesUtils.applyIfPresent(subset, row, subset -> modelAdder.setSubset(CgmesSubset.valueOf(subset))); |
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.
Either we decide to stay with the long name for the python dataframe (eg. EQUIPMENT
, STEADY_STATE_HYPOTHESIS
) or we want the condensed form (EQ
, SSH
) and a new method must be added (here or in CgmesSubset) to get the right CgmesSubset from the condensed String.
@@ -453,9 +453,12 @@ def test_geo_data(): | |||
|
|||
pd.testing.assert_frame_equal(n.get_extensions('linePosition'), line_expected) | |||
|
|||
|
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.
A test can be added here to try to get/add/remove a 'cgmesMetadataModels' extension on a network.
The creating dataframe can be as follow :
pd.DataFrame.from_records(index='id', columns=['id', 'subset', 'description', 'version', 'modelingAuthoritySet', 'profiles', 'dependentOn', 'supersedes'], data=[('sshId', 'STEADY_STATE_HYPOTHESIS', 'SSH description', 1, 'http://powsybl.org', 'steady-state-hypothesis', 'ssh-dependency1/ssh-dependency2', '')])
Signed-off-by: Naledi EL CHEIKH <naledi.elcheikh@rte-france.com>
Signed-off-by: Naledi EL CHEIKH <naledi.elcheikh@rte-france.com>
Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
Fixes #761
What kind of change does this PR introduce?
Introduces the storage of Cgmes Metadata via an extension
What is the current behavior?
#761
Does this PR introduce a breaking change or deprecate an API?