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

should all nodes be in edges (if edges exist)? #66

Open
Remi-Gau opened this issue Sep 10, 2022 · 5 comments
Open

should all nodes be in edges (if edges exist)? #66

Remi-Gau opened this issue Sep 10, 2022 · 5 comments

Comments

@Remi-Gau
Copy link
Contributor

All nodes mentioned in edges should refer to an existing node, but I am not sure about the other way around: have some nodes that exist but not mentioned in edges and that would thus be ignore when running the model.

Sounds practical from a user perspective to be able to develop some models where not all nodes are used while tweaking things around.

But validation could possibly throw a warning.

@effigies
Copy link
Contributor

Definitely seems warn-worthy.

@Remi-Gau
Copy link
Contributor Author

should I just add a validator in the BIDSStatsModel class ?

@effigies
Copy link
Contributor

I guess those could warn instead of raising exceptions, but if they aren't easy to skip, it will make it annoying to program with this class, as every copy will emit a new warning.

@Remi-Gau
Copy link
Contributor Author

ah true!

so maybe something like validate method on the Edge class?

@effigies
Copy link
Contributor

No, I think it still needs to operate on the whole model, as it needs to know about both nodes and edges. I can see a couple approaches.

  1. An explicit check() method or function that will emit warnings (could have a strict mode to make errors)
  2. Have an explicitly warning subclass of BIDSStatsModel that implements the validation. The caller then chooses which to use for parsing.

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