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

[REF] refactor qmri metadata to add JSON in root folder #337

Merged
merged 7 commits into from
Mar 15, 2023

Conversation

Remi-Gau
Copy link
Contributor

@Remi-Gau Remi-Gau commented Oct 21, 2022

relates to:

validation failure is expected until PR 1546 of the validator is merged

modifying those datasets should act as a "regresssion test" for the bug:

@Remi-Gau
Copy link
Contributor Author

pinging @agahkarakuzu to keep him in the loop on this.

qmri_megre/MEGRE.json Outdated Show resolved Hide resolved
@agahkarakuzu
Copy link
Contributor

Hi @Remi-Gau sorry I could get back to this just now. Just to make sure that I understand, the purpose it to collect parameters that do not change across file collection under one top json file that has the name of the suffix right?

I remember we discussed something like this to remove redundancy, but was not an option at the time we were developing the standard.

qmri_megre/MEGRE.json Outdated Show resolved Hide resolved
qmri_mtsat/MTS.json Outdated Show resolved Hide resolved
qmri_vfa/VFA.json Show resolved Hide resolved
@Remi-Gau
Copy link
Contributor Author

Hi @Remi-Gau sorry I could get back to this just now. Just to make sure that I understand, the purpose it to collect parameters that do not change across file collection under one top json file that has the name of the suffix right?

I remember we discussed something like this to remove redundancy, but was not an option at the time we were developing the standard.

Well yes and no.

Because of the Inheritance principle, users can still do that (meaning they can put json files in the root folder that contain metadata shared across files downstream).

But currently if they do so then the validator complains and does not recognized those files as valid.

So I am modifying those datasets to demonstrate the bug and create a regression test.

I have not given much thought on how to allocate metadata in the root folder and just tied to dump all common metadata there. Apparently I made some mistake.

Also I agree that it may be wiser to put only in there the metadata that make the most sense from a qmri perspective given that those datasets are pretty much a "first line of defense" when it comes to "good" small examples.

@Remi-Gau
Copy link
Contributor Author

Another thing. There is a tacit rule that bids examples datasets can diverge from the real datasets they originated from. So my changes here do not have to be reflected in the ones you have on OSF.

@Remi-Gau
Copy link
Contributor Author

Will adress other changes when I get access to a computer. So in a few days.

@agahkarakuzu
Copy link
Contributor

I see, thank you so much Remi!

@Remi-Gau
Copy link
Contributor Author

@agahkarakuzu I updated which metadata are in the root of the datasets.

From my reading of the qmri section, there is nothing that jumps at me that could be unclear regarding how to spread the metadata around.

@Remi-Gau
Copy link
Contributor Author

@sappelhoff if we mostly care about the master branch of the validator passing then this should be good to go as well, no?

@sappelhoff
Copy link
Member

@sappelhoff if we mostly care about the master branch of the validator passing then this should be good to go as well, no?

yes, the stable branch will fail until MOTION additions are released on the validator repo.

If this is ready, let's merge it as well 👍 will someone mirror the changes to the real data?

@Remi-Gau
Copy link
Contributor Author

I remember that we had a soft agreement that datasets in the examples can start diverging from their uostream dataset and that we do not have to keep them in synch.

But if @agahkarakuzu feels this would be anice addition to the real examples on osf I could give it a go.

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.

4 participants