-
Notifications
You must be signed in to change notification settings - Fork 4
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 broken template FCSVs #179
Conversation
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.
Couple of things that we probably want to consider as well.
-
Right now all templates are for humans, but I think we are close to adding in non-human templates as well. Should this be a separate test (might be related to point 2) or should this single test be more agnostic?
-
How would we handle testing in the event that the fiducials are not named the same across species?
I think this should either be a separate test or this test should be extended when we add non-human templates.
The validation logic in |
Fair - in this case, how would you feel about renaming
Ahh yeah, been a while - forgot thats there. And we know it works for NHPs at least (I think fiducials are named the same). Looks good to me otherwise. |
Done |
Proposed changes
Add a test for template FCSV validity and (eventually) fix the broken templates
Types of changes
What types of changes does your code introduce? Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you are unsure about any of the choices, don't hesitate to ask!black
with the-l 79
flag.Notes
All PRs will undergo the unit testing before being reviewed. You may be requested to explain or make additional changes before the PR is accepted.
PR template was adopted from appium