-
Notifications
You must be signed in to change notification settings - Fork 10
Improve framework for validation #204
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
Conversation
|
Note: add |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Todos
|
CodSpeed Performance ReportMerging #204 will degrade performances by 23.19%Comparing Summary
Benchmarks breakdown
|
…evel subfunctions
|
As discussed in #309, typing is failing due to an issue in python-typeshed, which should be resolved when python/typeshed#14597 is merged and released. I don't know that we need to hold this PR up for that though. |
| raise ValueError(f"Repeated edges found in data:\n{invalid_edges}") | ||
|
|
||
|
|
||
| class ValidationConfig(BaseModel): |
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.
Thoughts on adding nodes/edges to this config and then only passing one argument to the read/write functions? I assume you considered it and rejected, and I don't have a strong opinion, but bringing it up just in case
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.
Ben and I had created a separation in our brains between the graph (as mandatory data) and optional attribute data. We hadn't considered putting the graph into the validation config, but I like it. I'll make that change.
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.
As discussed elsewhere, could consider rolling the structure into the same argument but up to you
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.
I decided to leave structure out, but consolidated all the data once into the validation config
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 a few minor things about the API, but I love it in general! 🦖
…ptional split) with all the options in the validation config
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.
🌴 🎋 🎄 🌳 🌲
Proposed Change
This PR works towards the validation framework described here: #152 (comment). It will ultimately incorporate some of the validation functions contributed in #180.
New checks that have been implemented so far
Types of Changes
What types of changes does your code introduce? Delete those that do not apply.
Which topics does your change affect? Delete those that do not apply.
Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
Further Comments