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

DWI derivatives: Compulsory "parameter" entity #52

Merged
merged 4 commits into from
Sep 12, 2022

Conversation

Lestropie
Copy link
Collaborator

Realised this discrepancy while using the demonstrative examples for another purpose. Would like to make a reasonable decision on this prior to the data being presented elsewhere.

Will need to be resolved against #51.

While the specification states that the "parameter" entity must always be defined---even if all model parameters are encoded within a single image, in which case value "all" must be used---two of the demonstrative examples did not obey such.
@Lestropie Lestropie self-assigned this Aug 9, 2022
@arokem
Copy link
Collaborator

arokem commented Aug 10, 2022

With #51 merged, I think all mentions of parameter here should be changed to param.

Remind me again why tensor components need the all modifier?

Resolves #52 (making "parameter" entity compulsory) against #51 (changing key for that parameter from "parameter" to "param").
@Lestropie
Copy link
Collaborator Author

Remind me again why tensor components need the all modifier?

By making "_param-all" compulsory for any model where all components are encased within a single file, it makes validation far easier. This makes more sense on the premise that #46 is merged. Any file with suffix "model" would always necessitate the definition of entities "model" and "param". If such models are allowed to omit the "param" entity, then the validator (and potentially software reading from such data that attempts to be somewhat model-agnostic) would need to be able to look at other contents in the directory to make sure that there are no other files belonging to the same model.

Happy to have that debate, I certainly had it internally, but it warrants its own issue listing in that case.

Conflicts:
	src/05-derivatives/05-diffusion-derivatives.md
@Lestropie Lestropie merged commit 143d1cc into bep-016 Sep 12, 2022
@Lestropie Lestropie deleted the compulsory_parameter_entity branch September 12, 2022 21:00
Lestropie added a commit that referenced this pull request Sep 13, 2022
In cases where all model parameters can be encapsulated in a single NIfTI image, revert back to the case where the "param" entity is omitted from the file name.
Reverts #52.
@Lestropie Lestropie mentioned this pull request Sep 13, 2022
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.

2 participants