-
Notifications
You must be signed in to change notification settings - Fork 45
Drop support for CIM14 #3375
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
base: main
Are you sure you want to change the base?
Drop support for CIM14 #3375
Conversation
Signed-off-by: Giovanni Ferrari <[email protected]>
Signed-off-by: Giovanni Ferrari <[email protected]>
Signed-off-by: Giovanni Ferrari <[email protected]>
Signed-off-by: Giovanni Ferrari <[email protected]>
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.
Thanks for your work! I left a few comments and some questions for @rcourtier :)
...ersion/src/main/java/com/powsybl/cgmes/conversion/elements/SynchronousMachineConversion.java
Show resolved
Hide resolved
cgmes/cgmes-model/src/test/java/com/powsybl/cgmes/model/CgmesOnDataSourceTest.java
Outdated
Show resolved
Hide resolved
cgmes/cgmes-model/src/test/java/com/powsybl/cgmes/model/WindingTypeTest.java
Show resolved
Hide resolved
...-conversion/src/test/java/com/powsybl/cgmes/conversion/test/ImportExportPerformanceTest.java
Outdated
Show resolved
Hide resolved
cgmes/cgmes-model/src/main/java/com/powsybl/cgmes/model/triplestore/CgmesModelTripleStore.java
Outdated
Show resolved
Hide resolved
...onversion/src/test/java/com/powsybl/cgmes/conversion/test/export/CgmesExportContextTest.java
Show resolved
Hide resolved
...ersion/src/main/java/com/powsybl/cgmes/conversion/elements/SynchronousMachineConversion.java
Outdated
Show resolved
Hide resolved
...ersion/src/main/java/com/powsybl/cgmes/conversion/elements/SynchronousMachineConversion.java
Show resolved
Hide resolved
...-conversion/src/test/java/com/powsybl/cgmes/conversion/test/ImportExportPerformanceTest.java
Outdated
Show resolved
Hide resolved
cgmes/cgmes-model-test/src/main/java/com/powsybl/cgmes/model/test/Cim14SmallCasesCatalog.java
Show resolved
Hide resolved
cgmes/cgmes-model-test/src/main/java/com/powsybl/cgmes/model/test/Cim14SmallCasesCatalog.java
Show resolved
Hide resolved
...onversion/src/test/java/com/powsybl/cgmes/conversion/test/export/CgmesExportContextTest.java
Show resolved
Hide resolved
cgmes/cgmes-model/src/main/java/com/powsybl/cgmes/model/CgmesNamespace.java
Outdated
Show resolved
Hide resolved
cgmes/cgmes-model/src/main/java/com/powsybl/cgmes/model/WindingType.java
Outdated
Show resolved
Hide resolved
cgmes/cgmes-model/src/main/java/com/powsybl/cgmes/model/triplestore/CgmesModelTripleStore.java
Outdated
Show resolved
Hide resolved
cgmes/cgmes-model/src/test/java/com/powsybl/cgmes/model/CgmesOnDataSourceTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Giovanni Ferrari <[email protected]>
Thanks for the changes, I left new comments too (things I missed the first time). There are also 2 test resources files that you can delete: empty_cim14_EQ.xml and empty_cim14_SV.xml. There are also tests where "cim14" still appears, namely:
(the cgmes-conversion module test resources cim14 folder should be deleted too) In the OperationalLimitConversion class, there is a comment mentioning CIM14 (line 52). The file invalidContent_EQ in the cgmes-model-test still mentions "cim14" as well. |
CgmesNamingStrategyTest and EquipmentExportTest use files nordic32.xiidm and ieee14.xiidm from cim14 folder, but the files are in XIIDM format. What should I do ? CgmesOnDataSourceTest.testExists() test is using empty_cim14_EQ.xml to test that CIM14 is not supported OperationalLimitConversion what should I do with this check ?
|
In |
cgmes/cgmes-model/src/test/java/com/powsybl/cgmes/model/CgmesOnDataSourceTest.java
Outdated
Show resolved
Hide resolved
cgmes/cgmes-model/src/test/java/com/powsybl/cgmes/model/CgmesOnDataSourceTest.java
Outdated
Show resolved
Hide resolved
cgmes/cgmes-model/src/test/java/com/powsybl/cgmes/model/CgmesOnDataSourceTest.java
Outdated
Show resolved
Hide resolved
...n/java/com/powsybl/cgmes/conversion/elements/transformers/AbstractTransformerConversion.java
Outdated
Show resolved
Hide resolved
You can remove the files (the whole folder cim14 actually) and the corresponding tests in
You can keep empty_cim14_EQ.xml as you said to highlight the fact that CIM14 is not supported.
You can consider that limitSubclass is not null: if (limitSubclass.equals(CgmesNames.ACTIVE_POWER_LIMIT) || limitSubclass.equals(CgmesNames.APPARENT_POWER_LIMIT) || limitSubclass.equals(CgmesNames.CURRENT_LIMIT)) {
if (terminalId != null) {
... |
Signed-off-by: Giovanni Ferrari <[email protected]>
Signed-off-by: Giovanni Ferrari <[email protected]>
...nversion/src/main/java/com/powsybl/cgmes/conversion/elements/OperationalLimitConversion.java
Outdated
Show resolved
Hide resolved
...nversion/src/test/java/com/powsybl/cgmes/conversion/test/export/CgmesNamingStrategyTest.java
Show resolved
Hide resolved
Signed-off-by: Giovanni Ferrari <[email protected]>
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.
Thank you for your work!
// Support for CIM14, all limits are assumed to be current | ||
limitSubclass = CgmesNames.CURRENT_LIMIT; | ||
} | ||
if (limitSubclass.equals(CgmesNames.ACTIVE_POWER_LIMIT) || limitSubclass.equals(CgmesNames.APPARENT_POWER_LIMIT) || limitSubclass.equals(CgmesNames.CURRENT_LIMIT)) { |
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 would be cleaner this way:
if (limitSubclass.equals(CgmesNames.ACTIVE_POWER_LIMIT) || limitSubclass.equals(CgmesNames.APPARENT_POWER_LIMIT) || limitSubclass.equals(CgmesNames.CURRENT_LIMIT)) { | |
if (CgmesNames.ACTIVE_POWER_LIMIT.equals(limitSubclass) || CgmesNames.APPARENT_POWER_LIMIT.equals(limitSubclass) || CgmesNames.CURRENT_LIMIT.equals(limitSubclass)) { |
@@ -30,8 +30,7 @@ public class SynchronousMachineConversion extends AbstractReactiveLimitsOwnerCon | |||
public SynchronousMachineConversion(PropertyBag sm, Context context) { | |||
super(CgmesNames.SYNCHRONOUS_MACHINE, sm, context); | |||
String type = p.getLocal("type"); | |||
// CIM14 uses Type.condenser, CIM16 and CIM100 use Kind.condenser | |||
isCondenser = type != null && type.endsWith(".condenser"); | |||
isCondenser = type.equals("SynchronousMachineKind.condenser"); |
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.
isCondenser = type.equals("SynchronousMachineKind.condenser"); | |
isCondenser = "SynchronousMachineKind.condenser".equals(type); |
Arguments.of("rdf cim16 not cim14", "empty_cim16_EQ.xml", "14", false), | ||
Arguments.of("rdf cim14 not cim16", "empty_cim14_EQ.xml", "16", false) | ||
); | ||
Arguments.of("EQ cim14", "empty_cim14_EQ.xml", false), |
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.
Just to be clear why we have kept this test:
Arguments.of("EQ cim14", "empty_cim14_EQ.xml", false), | |
Arguments.of("EQ cim14", "empty_cim14_EQ.xml", false), // CIM14 is not supported |
@@ -41,23 +37,6 @@ | |||
*/ | |||
class CgmesNamingStrategyTest extends AbstractSerDeTest { | |||
|
|||
@Test |
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.
Shouldn't we keep an equivalent of the deleted tests, but in another CIM version?
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 with NamingStrategyFactory.CGMES
has been kept (testExportUsingCgmesNamingStrategyMicroGrid()
). It was deemed complete enough to test the naming strategy.
@@ -199,145 +194,6 @@ void microGridCreateEquivalentInjectionAliases() throws IOException, XMLStreamEx | |||
assertTrue(compareNetworksEQdata(expected, actual)); | |||
} | |||
|
|||
@Test | |||
void nordic32() throws IOException, XMLStreamException { |
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.
Shouldn't we also try to keep these tests?
(On a note side, there's still a CIM16 nordic32.xml
file in the resources which doesn't seem to be called.)
Signed-off-by: Giovanni Ferrari <[email protected]>
|
Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
Fixes #3290
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)
CgmesModel.CIM_14_NAMESPACE
,CgmesModel.CIM_14
,CgmesOnDataSource.existsCim14()
: now it would have always returnedfalse
,CgmesModel.Cim.hasProfiles()
: now it would have always returnedtrue
.CgmesModel
interface was also modified. The methodmodelProfiles
is no longer a default method, meaning that you have now to implement it if you designed your own implementation of the interface.CIM_LIST
from theCgmesNamespace
class has gone fromList.of(CIM_14, CIM_16, CIM_100)
toList.of(CIM_16, CIM_100)