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

Resample to n_vols for sampling_rate == 'TR' #713

Merged
merged 14 commits into from
Apr 12, 2021
Merged

Resample to n_vols for sampling_rate == 'TR' #713

merged 14 commits into from
Apr 12, 2021

Conversation

adelavega
Copy link
Collaborator

@adelavega adelavega commented Apr 5, 2021

WIP Fix to #712

  • Keep track of n_vols in RunInfo (might be a good idea anyways)
  • Use n_vols to compute the number of resample volumes in resample if sampling_rate == TR
  • To do so, need to propagate the TR value one level up (instead of computing true sampling_rate at Collection level).

@adelavega
Copy link
Collaborator Author

We already have a test which catches the situation in which round does not return the expected index length.

I'm going to implement the special case scenario instead.

@effigies
Copy link
Collaborator

effigies commented Apr 6, 2021

Failing test introduced in #365.

@adelavega adelavega changed the title Round instead of ceil Resample to n_vols for sampling_rate == 'TR' Apr 6, 2021
bids/variables/io.py Outdated Show resolved Hide resolved
bids/variables/io.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is currently testing the edge case you brought up? Or can you comment on the lines that do?

Otherwise looks good.

bids/variables/io.py Outdated Show resolved Hide resolved
Co-authored-by: Chris Markiewicz <[email protected]>
@adelavega
Copy link
Collaborator Author

@effigies yep, still need to add a test. turned out to be a bit cumbersome but it seemed like adding n_vols to run_info was best, as it may end up being a good thing to have around anyways

@adelavega
Copy link
Collaborator Author

@effigies I added a test that sort of gets at it by setting a scan_length that is off by .1 (simulating the rounding error you sometimes see IRL). It's not the most robust test since it's not being read directly from the dataset. I think to do that however, we'd need to a new dataset with a TR below 1 and with some real images to read n_vol off. Not sure if worth it.

@adelavega
Copy link
Collaborator Author

This seems to be working for me. Care to review @tyarkoni ?

Copy link
Collaborator

@tyarkoni tyarkoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

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