Skip to content
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

Check - all nodes must be used in a link? #5

Open
ghost opened this issue Oct 18, 2022 · 7 comments
Open

Check - all nodes must be used in a link? #5

ghost opened this issue Oct 18, 2022 · 7 comments

Comments

@ghost
Copy link

ghost commented Oct 18, 2022

From #1 (comment)

All nodes must be used in a link?

From #1 (comment)

I don't think we do want to say this. There may be edge cases where nodes are unattached to a link for good reason, and from a graph theory perspective floating nodes don't present a problem

@ghost
Copy link
Author

ghost commented Oct 18, 2022

The only use case I can think of so far is that Phase 1 includes building some nodes, and Phase 2 which isn't fully mapped or described yet is going to connect them.

[ I could also see one network operator running 2 totally independent networks that aren't actually connected (maybe on different sides of a country or something) but this check wouldn't cover that anyway.]

How about we do this but style it as a warning instead of an error?

It's possible that any occurrences of this are more likely to be a result of bad data mapping / entry than a real world edge case, and thus the warning would be helpful?

@lgs85
Copy link

lgs85 commented Oct 18, 2022

Yes, a warning would be helpful. @duncandewhurst suggested the following:

It's not a normative rule but it could be a useful 'sense check' with a message like:

Your data contains nodes that are not referenced by any links. You should check that this is not the result of an error in your mapping or data pipeline.

ghost pushed a commit that referenced this issue Oct 18, 2022
ghost pushed a commit that referenced this issue Oct 18, 2022
ghost pushed a commit that referenced this issue Oct 18, 2022
ghost pushed a commit that referenced this issue Oct 18, 2022
@duncandewhurst
Copy link
Contributor

Agree it should be a warning not an error. In the supply-side research we saw examples of datasets that were effectively lists of nodes without information about links.

@ghost ghost self-assigned this Oct 19, 2022
@duncandewhurst
Copy link
Contributor

Looks good on the testing server. Just need to update the table to have the correct columns per the additional checks spreadsheet from Open-Telecoms-Data/cove-ofds#13 (comment)

@duncandewhurst
Copy link
Contributor

Testing on 2022-11-08 with this file, CoVE fails to report the unreferenced node.

@ghost
Copy link
Author

ghost commented Nov 29, 2022

I can't actually find this in spread sheet any more, but I think this is now on live and ready to test?

Test file: https://raw.githubusercontent.com/Open-Telecoms-Data/lib-cove-ofds/main/tests/fixtures/pythonvalidate/node_not_used_in_any_spans_1.input.json

@duncandewhurst
Copy link
Contributor

duncandewhurst commented Dec 1, 2022

It's the first check in the spreadsheet, but at some point we renamed it from 'node references' to 'unreferenced nodes' - I've updated the spreadsheet now.

I'm still not seeing a failure reported when testing with the file shared in my previous comment: https://ofds.cove.opendataservices.coop/data/7ea82278-5f8e-439a-9407-0103e75e7331

Could that be because there are two nodes with the same .id in that file?

Edit: It's working OK with the test file that you shared, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants