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

Test validation of cyclic graph + local consts, doc failures #224

Closed
wants to merge 3 commits into from

Conversation

acl-cqc
Copy link
Contributor

@acl-cqc acl-cqc commented Jun 27, 2023

  • test_cyclic creates a cyclic DFG, and this validates :-(. We don't have anything in validation to look for this.
  • test_local_const creates a graph with a LoadConstant node that is not in the causal cone of the Input. This passes validation, because the DfsPostOrder used in validate_children_dag currently traverses both ways along edges :-(, I think validation should fail here, and that the fix is in portgraph.
    • test_local_const then goes on to add the missing edge, and that still passes, but if we fixed the traversal to only go forwards, validate_children_dag would then fail as it wouldn't have visited the Const node (but would count it in the children); we'd then need the fix detailed inside validate_children_dag

I'd love to turn these into "expected-fail" tests but that looks quite hard, best plan appears to be to use cargo-nextest

@acl-cqc acl-cqc force-pushed the tests/validation branch from 04a824c to 952e3da Compare June 27, 2023 17:14
@acl-cqc acl-cqc changed the base branch from main to tests/validate_ext_edge June 27, 2023 17:15
@acl-cqc acl-cqc changed the title Various tests, and various failures Test validation of cyclic graph + local consts, doc failures Jun 27, 2023
Base automatically changed from tests/validate_ext_edge to main June 28, 2023 10:51
@acl-cqc
Copy link
Contributor Author

acl-cqc commented Jun 28, 2023

Closing in favour of #227 and #228

@acl-cqc acl-cqc closed this Jun 28, 2023
@acl-cqc acl-cqc deleted the tests/validation branch August 3, 2023 10:41
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

Successfully merging this pull request may close these issues.

1 participant