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

TEST: dataset-level model spec retrieval #693

Merged
merged 8 commits into from
Apr 18, 2021
Merged

TEST: dataset-level model spec retrieval #693

merged 8 commits into from
Apr 18, 2021

Conversation

tyarkoni
Copy link
Collaborator

@tyarkoni tyarkoni commented Jan 5, 2021

Very minor test that verifies you can retrieve a model specification object for a BIDS-StatsModels analysis at levels past run (in this case, dataset). (I think black went a little crazy on the formatting, which was unintended, but the only substantive changes are the addition of test_get_dataset_level_model_spec.)

@effigies
Copy link
Collaborator

effigies commented Jan 6, 2021

Rebased to run tests.

@effigies
Copy link
Collaborator

effigies commented Jan 6, 2021

@tyarkoni I'm going to revert all style changes except test_get_dataset_level_model_spec, since there are errors in unrelated tests.

@tyarkoni
Copy link
Collaborator Author

tyarkoni commented Jan 6, 2021

Sounds good, sorry about that.

@effigies
Copy link
Collaborator

effigies commented Jan 6, 2021

Actually, I think the failures are related to the change in the model. Never mind.

@tyarkoni
Copy link
Collaborator Author

Ah, sorry—I forgot to run the other tests before pushing.

These failures actually suggest a deeper problem that will probably require more than just amending the tests. It looks like pybids doesn't currently broadcast higher-level metadata properly when there's more than one row per unit passed from the previous subject. E.g., if you go straight from run to dataset level, and you're passing 10 runs from 2 subjects, then at the dataset level you have 10 rows, but participants.tsv only contains 2 rows, so you need to broadcast the latter to have the same shape as the former. Will fix this when I can.

@tyarkoni
Copy link
Collaborator Author

@effigies tests are breaking because of #682; let's ignore that here and I'll deal with it separately (I think the issue is that the JSON changes @oesteban introduced to cause #683 and trigger the new test are breaking other things).

If you want to review this, the key changes are in this section. My earlier assessment was wrong; the issue here isn't broadcasting-related, it's that bad things happen when metadata is missing for some rows but not others (e.g., the rows propagated to the dataset level from runs --> sessions --> subjects carries information about the scanning sequence, but this is missing from rows ingested from participants.tsv). Things were already working fine in "long" format but were breaking in "wide"; I've now fixed it so it works both ways.

@oesteban
Copy link
Collaborator

Seems to be rooting from the new bids/tests/data/7t_trt/sub-01/ses-1/sub-01_ses-1_scans.json file. If doing that kind of thing is explicitly ruled out in the specs, that file can just be deleted.

I wonder if it would suffice to first merge #683 and use the hashablefy function on line 689 of the layout code where things break. Isn't #683 updated so that this error can be replicated there?

@tyarkoni
Copy link
Collaborator Author

@effigies we should probably merge this before mucking around further with the design matrix construction stuff... the test failures look like a CI glitch, and are otherwise passing.

@effigies effigies merged commit b6cd0f6 into master Apr 18, 2021
@effigies effigies deleted the add-dataset-test branch April 18, 2021 12:06
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.

3 participants