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
fix(onto validation): correctly identify circular dependencies (DEV-769) #192
Conversation
delete circle_detection_validate.py
make freeze-requirements
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.
LGTM, there are already tests that test the expected behaviour, right?
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.
Very nice!
I second Irina that this should be tested. If it already is, even better! Otherwise it would be good to have a couple of unit tests for the circular dependency method.
One thing though: I think the PR title could be a bit more descriptive (as this is used for the changelog, so it should be understandable for someone who doesn't know the code). Rather than naming the function that is improved, it should mention the functionality that is improved.
knora/dsplib/utils/onto_validate.py
Outdated
# transform the dependencies into a graph structure | ||
graph = nx.MultiDiGraph() | ||
for start, cards in dependencies.items(): | ||
for edge, targets in cards.items(): | ||
for target in targets: | ||
graph.add_edge(start, target, edge) | ||
|
||
# check circles if they have all '0-1' or '0-n' | ||
errors: set[str] = set() | ||
circles = list(nx.simple_cycles(graph)) | ||
for circle in circles: | ||
for index, resource in enumerate(circle): | ||
target = circle[(index+1) % len(circle)] | ||
for property, targets in dependencies[resource].items(): | ||
if target in targets: | ||
prop = property | ||
if cardinalities[resource][prop] not in ['0-1', '0-n']: | ||
errors.add(f'Resource "{resource}", property "{prop}"') |
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 is a really neat solution for the problem, congratulations!
- base properties derived from hasLinkTo - user-defined properties derived from the above
I added tests, and refactored the code to make it better testable. |
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.
LGTM. It is a good idea to do it in a separate ontology. This keeps things clean and tidy.
Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>
Kudos, SonarCloud Quality Gate passed!
|
resolves DEV-769