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

Api File Naming #269

Merged
merged 5 commits into from
Jun 25, 2020
Merged

Api File Naming #269

merged 5 commits into from
Jun 25, 2020

Conversation

36000
Copy link
Collaborator

@36000 36000 commented Jun 23, 2020

Show session and subject in all file names. Moved common inclusions for file names into fname function. Some pep8 fixes.

@pep8speaks
Copy link

pep8speaks commented Jun 23, 2020

Hello @36000! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-06-24 20:13:40 UTC

@36000 36000 changed the title Api naming Api File Naming Jun 23, 2020
@36000
Copy link
Collaborator Author

36000 commented Jun 23, 2020

resolves #258

@36000
Copy link
Collaborator Author

36000 commented Jun 24, 2020

@arokem if the dmriprep files already have the sub-XX_sess-XX prefix like in BIDs format shown below, then we do not need this pull request, the api already grabs this prefix from the file.
image
Should I use this PR to add in the sub-XX_sess-XX prefix if it doesn't exist, or should I drop this aspect of this PR all together?

@arokem
Copy link
Collaborator

arokem commented Jun 24, 2020

Oh yeah - let's leave the output like the input. But in our own work, let's make sure the inputs are fully spelled out.

I think that means we can close this?

@36000
Copy link
Collaborator Author

36000 commented Jun 24, 2020

No on closing. I also moved common inclusions for file names into the fname function. This won't change functionality but makes the code nicer. For example, Instead of having this in every function:

odf_model = self.tracking_params['odf_model']

directions = self.tracking_params['directions']

...

fname = fname + (

                f'_space-RASMM_model-{odf_model}'

                f'_desc-{directions}'

            )

You can instead use the include_track argument when you want to include that information in the file name:
self._get_fname(row, suffix, include_track=True)

@36000
Copy link
Collaborator Author

36000 commented Jun 24, 2020

I rebased so now this PR only includes the convenient function and some pep8 fixes. However, #268 would use this function, so we should not merge both without updating and rebasing one on top of the other.

@arokem
Copy link
Collaborator

arokem commented Jun 25, 2020

LGTM! I love it when a PR removes more lines of code than it adds :-)

@arokem arokem merged commit a70a267 into yeatmanlab:master Jun 25, 2020
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.

None yet

3 participants