-
Notifications
You must be signed in to change notification settings - Fork 20
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
Minor bug fixes #37
base: master
Are you sure you want to change the base?
Minor bug fixes #37
Conversation
@@ -98,6 +98,9 @@ def cosine_to_orientation(iop): | |||
express the direction you move, in the DPCS, as you move from row to row, | |||
and therefore as the row index changes. | |||
|
|||
Notes: | |||
Modified Yarik's original solution from https://stackoverflow.com/a/45469577 to use the argmax for increaesd flexibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~~hm -- was it mine? says user6867490 there. ~~
note the typo
Modified Yarik's original solution from https://stackoverflow.com/a/45469577 to use the argmax for increaesd flexibility. | |
Modified Yarik's original solution from https://stackoverflow.com/a/45469577 to use the argmax for increased flexibility. |
I will bolt on codespell now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually -- I don't really see a reason for this note here at all. But I feel it is time to add tests for this function to ensure that it operates as expected... ATM can't quite grasp the ramification of the change, in particular for those cases where before a RuntimeError would have been raised.
parser.add_argument( | ||
"experiment_id", | ||
help="Experiment ID assigned by NDA for collection. Requred for fMRI studies", | ||
metavar='EXPERIMENT_ID') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, I wonder how we managed to upload before without this?
so we were taking experiment id from metadata and it worked on our few uploads I participated in.
Do you know if it may be recent or could be optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind -- it is in the notes of README.md , so good to have it here in the code now but we would need to remove that note from README.md here. care to do that @ljchang ?
Not sure if this project is still being maintained, but I wanted to share a few small changes I made so that I was able to successfully upload my bids formatted data to the NDA. It looks like a few other people have experienced similar problems, so hopefully somebody will find this useful.
cosine_to_orientation()
function more flexible for accommodating a range of values. It looks like there was already some discussion about this Fixes and improvements (by mtnhuck) #13.