Donders-Institute / bidscoin

BIDScoin converts your source-level neuroimaging data to BIDS
https://bidscoin.readthedocs.io
GNU General Public License v3.0
130 stars 35 forks source link

Bidsmap schema doesn't require meta key in plugin #225

Closed mateuszpawlik closed 9 months ago

mateuszpawlik commented 9 months ago

Describe the bug I tried to use my bidsmap file with BIDScoin 4.2.1 I looked also at the schema, if I have to update anything. I managed also to execute a validator on my map and it passed. However, BIDScoin errored out (see Screenshots).

I think, it expects the map key for the plugins. See line https://github.com/Donders-Institute/bidscoin/blob/45108049f22502a3a34df57b070dc36a52593123/bidscoin/plugins/dcm2niix2bids.py#L437

After adding this key with the default values to the plugin section, BIDScoin succeeded.

To reproduce I can point to failed pipeline at the moment: https://gitlab.com/ccns/neurocog/neurodataops/anc/pipelines/anc-data-acquisition/-/pipelines/1173572096

Expected behavior Ideally, I would be prompted that this key is missing in my bidsmap file. The input map could be validated against the schema before anything else happens. The schema may need to be adjusted with all required keys.

Screenshots https://gitlab.com/ccns/neurocog/neurodataops/anc/pipelines/anc-data-acquisition/-/jobs/6152242679#L357

Software version

marcelzwiers commented 9 months ago

Have you tried to run:

bidscoin -t path_to_your_template
mateuszpawlik commented 9 months ago

No, I missed that or forgot. Thank you. I'll try it out and come back to you.

marcelzwiers commented 9 months ago

It's true that the schema doesn't fully check the plugin options (that's not really doable), but this one it should have. I now added it :-)

Thank for reporting!

mateuszpawlik commented 9 months ago

I executed the command with version 4.2.1 and my bidsmap was fine. It didn't report the missing meta key.

marcelzwiers commented 9 months ago

Ow, that's not good. I now fixed it so it won't crash anymore

mateuszpawlik commented 9 months ago

I installed the newest BIDScoin from master. It doesn't complain on this meta key. Do you execute json schema validation for the given map?

marcelzwiers commented 9 months ago

No, I only validate against the schema in my pytests

marcelzwiers commented 9 months ago

https://github.com/Donders-Institute/bidscoin/blob/master/tests/test_heuristics.py

mateuszpawlik commented 9 months ago

Wouldn't it be great to validate the input map. This would help tremendously, especially when migrating to a newer version of the schema. Now, it's trial end error + looking up your template maps.

For example, I executed your test including my map and it failed on unzip key that I didn't know about.

marcelzwiers commented 9 months ago

Ow really, that's a bug, I thought that in my code unzip was optional. But sure, I can add a schema validation somewhere in the non-pytest code

marcelzwiers commented 9 months ago

Would bidsoin -t path/to/template do or do you mean every time a bidsmap is loaded?

marcelzwiers commented 9 months ago

Btw, unzip is required according to the json schema, and when I edit a template in e.g. Pycharm or VS code, it automatically shows up that unzip is required?

mateuszpawlik commented 9 months ago

Would bidsoin -t path/to/template do or do you mean every time a bidsmap is loaded?

It depends how do you intend bidscoin to be used with custom templates. If I use it with --template option I would expect that it errors out on invalid input and not expect it to be correct.

mateuszpawlik commented 9 months ago

Btw, unzip is required according to the json schema, and when I edit a template in e.g. Pycharm or VS code, it automatically shows up that unzip is required?

Do you have any VSCode setup for that? I don't find it in the project. This is another way to solve it. But I personally think, that validating the input is a good thing to do.

marcelzwiers commented 9 months ago

I'm more of a PyCharm user and I thought it also worked in VS code. But I just tried and indeed it doesn't. One more reason to stick with PyCharm :-)

Anyhow, I now added a schema test when loading a template bidsmap...

marcelzwiers commented 9 months ago

image

Just to make you jealous ;-)

mateuszpawlik commented 9 months ago

I would error out if the map is not valid. Otherwise it seems to work. I tried it with bidsmap -t my_map.yaml