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

[HOTFIX] Fix ``UnboundLocalError`` in utils.bids #285

Merged
merged 7 commits into from Jan 24, 2019

Conversation

Projects
None yet
3 participants
@oesteban
Copy link
Contributor

oesteban commented Jan 19, 2019

oesteban added some commits Jan 19, 2019

@oesteban

This comment has been minimized.

Copy link
Contributor Author

oesteban commented Jan 19, 2019

Can you make sure this is correct, @emdupre?

oesteban added some commits Jan 19, 2019

@emdupre

This comment has been minimized.

Copy link
Collaborator

emdupre commented Jan 19, 2019

Thanks for the quick fix !!

One concern here, and likely how the problem arose: dual echo data is considered a special case in multi-echo, and you would not make an optimal combination for it or calculate a T2* map, etc. Requesting one change for that:

Show resolved Hide resolved niworkflows/utils/bids.py Outdated
Update niworkflows/utils/bids.py
Co-Authored-By: oesteban <code@oscaresteban.es>
@emdupre

This comment has been minimized.

Copy link
Collaborator

emdupre commented Jan 19, 2019

Actually, I modified some data locally to be dual echo and I'm still not encountering the reported problem (neither with the original code nor your branch).

I looked at the error again and I'm still not sure how you'd hit that condition unless you didn't have BOLD data, but from his call it looks like it was BIDS validated with recognized BOLD data... Curious to understand what happened, here. Either way, I think your re-write is safer.

You are right, though, that they may want both files returned (depending on what they're doing with the dual echo). I'll make one more suggested change, sorry !

Show resolved Hide resolved niworkflows/utils/bids.py Outdated
@emdupre

This comment has been minimized.

Copy link
Collaborator

emdupre commented Jan 20, 2019

Ok, I've updated this to just return dual echos as if they were single-echo sequences. I believe my original reasoning was that even as a dual-echo sequence, there are certain facets which are better considered in a multi-echo context (such as coregistration), but that brings up some larger issues that I don't think are directly related, here.

Of note: I ran the original code with the file tree reported in the error and can't reproduce the issue locally. Is anyone else able to ?

@oesteban

This comment has been minimized.

Copy link
Contributor Author

oesteban commented Jan 23, 2019

Hi @emdupre, I finally decided to run the extra mile, split the function and write some unit tests. Could you check if the new commit looks fine?

In particular, please let me know if you prefer an plain if/else here.

With your thumbs up I could go ahead and merge :)

@oesteban

This comment has been minimized.

Copy link
Contributor Author

oesteban commented Jan 24, 2019

I'm going to go ahead. I'll happily accept any PR improving this implementation if necessary.

@oesteban oesteban merged commit f8e9813 into maint/0.5 Jan 24, 2019

5 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test_pytest Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@oesteban oesteban deleted the oesteban-patch-1 branch Jan 24, 2019

@oesteban oesteban restored the oesteban-patch-1 branch Jan 24, 2019

@oesteban oesteban deleted the oesteban-patch-1 branch Jan 24, 2019

oesteban added a commit that referenced this pull request Jan 24, 2019

Merge pull request #285 from poldracklab/oesteban-patch-1
[HOTFIX] Fix ``UnboundLocalError`` in utils.bids
@emdupre

This comment has been minimized.

Copy link
Collaborator

emdupre commented Jan 25, 2019

So sorry for the radio silence ! I'll look into this tomorrow, but from the quickest review it seems great 👍 I'll open a PR if I have any concerns :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment