Feature/Check subsystem graphs in sanity check - #24
Conversation
JohannesWeinbrecht
left a comment
JohannesWeinbrecht
left a comment
There was a problem hiding this comment.
As stated, I think this PR needs to wait until #25 is done, so we can add some correct models and test the sanity check fix on those as well.
Only having tests of the sanity check on real models were we assert failure is not sufficient IMO.
| :return: True if the model is valid, else False. | ||
| """ |
There was a problem hiding this comment.
I do not understand why this doc string was removed.
There was a problem hiding this comment.
Restored the return documentation and updated the docstring to mention recursive subsystem validation.
| data = json.loads(data_str) | ||
| core_model = CoreModel.import_dict(data) | ||
|
|
||
| self.assertFalse(core_model.sanity_check()) |
There was a problem hiding this comment.
We can keep the old models as a regression test just to make sure, but before merging I would like to generate some new, correct GDF models, with the (hopefully soon) fix in #25 (Issue: #16) and add those as tests as well, were the sanity actually correctly approves on a larger model.
The current state of only having sanity check tests on actual models where failure is asserted is IMO not sufficient.
There was a problem hiding this comment.
Keeping the old models as regression tests makes sense, but we should also add tests with newly generated, correct GDF models where the sanity check is expected to pass.
At the moment, I’m blocked on generating those models because although my VM is up and running, PowerFactory is no longer working on my side. Once that is resolved, I can generate the new models using the fix from #25 / #16 and add them as additional sanity-check tests.
|
Added a fresh PowerFactory-derived IEEE Std 399 GDF as a positive regression case. sanity_check() now passes after reload, while the existing IEEE39 fixtures still cover the failing cases. Tests: 6 passed in utils, 23 passed, 1 skipped in GDF. |
JohannesWeinbrecht
left a comment
JohannesWeinbrecht
left a comment
There was a problem hiding this comment.
LGTM
For documentation, I tested this sanity check on the following models
(newly converted using #25 and asserting sanity check yields true)
- IEEE39 clean
- IEEE39 clean flat
- IEEE39 pf default
- IEEE39 pf default flat
(assert sanity check yields false (old models from tests/models))
- IEEE39 flat
- IEEE39
3 participants
Updates CoreModel.sanity_check() so that subsystem graphs are checked recursively.
Changes:
Tests:
This builds on the sanity-check changes from #15.
Closes #17 and #8 (as it includes changes from #15)