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

Mismatch in -ctab option between json and tsv files #514

Closed
cjl2007 opened this issue Dec 26, 2019 · 4 comments · Fixed by #508
Closed

Mismatch in -ctab option between json and tsv files #514

cjl2007 opened this issue Dec 26, 2019 · 4 comments · Fixed by #508
Assignees
Labels
bug issues describing a bug or error found in the project

Comments

@cjl2007
Copy link

cjl2007 commented Dec 26, 2019

Hi - In previous versions of tedana, the input for --ctab was a tab delimitated text file (formatted just like the then output file, "comp_table_ica.txt"). If a user wanted to manually accept/reject components, they could edit the classification fields, and then rerun tedana, specifying the edited .txt file as the input for "--ctab"

In this latest version of tedana, however, the tab delimitated file output I am used to seeing has been replaced by a .json file called "ica_decomposition.json".

I assumed that I could edit the classification fields in this .json file, just as I had done with the .txt files, and rerun tedana using the edited .json file as the input for the --ctab option. This was not working.

I looked at the code briefly and it looked like the same bit of code that was used to load the classification table in earlier version of tedana, is still being used now ...

line 513 in tedana.py
comptable = pd.read_csv(ctab, sep='\t', index_col='component')

Which prompted me to try using a tab delimitated file instead, and this seems to have done the trick.

My question is whether this is expected behavior? It is not clear to me if the code just had not yet been updated to handle the new .json based component classification files, or if the user is expected to understand that a tab delimitated text file in a very specific format is expected. If the latter, then I think the documentation should be updated a bit to provide more information for users, because this wasn't obvious to me at first. Right now it just says,

--ctab = "File containing a component table from which to extract pre-computed classifications."

Thanks!
Chuck

@handwerkerd
Copy link
Member

This does look like a bug to me. @tsalo worked to convert the comptable to a BIDS compatible JSON file, which will have several longer-term advantages. As of now, it looks like the the comptable is saved in JSON format with the io.save_comptable function, but the load_comptable function isn't ever used to load the comptable. The acute problem might be addressed by swapping pd.read_csv with io.load_comptable on line 513 of tedana.py, but I'll defer to @tsalo.

@handwerkerd handwerkerd assigned handwerkerd and tsalo and unassigned handwerkerd Dec 27, 2019
@handwerkerd handwerkerd added bug issues describing a bug or error found in the project priority: high issues that would be really helpful if they were fixed already labels Dec 27, 2019
@tsalo
Copy link
Member

tsalo commented Dec 27, 2019

#508 handles this.

@handwerkerd handwerkerd removed the priority: high issues that would be really helpful if they were fixed already label Dec 27, 2019
@cjl2007
Copy link
Author

cjl2007 commented Dec 27, 2019

Great! Thanks for point that this is raised already in #508, sorry for the duplicate.

@cjl2007 cjl2007 closed this as completed Dec 27, 2019
@tsalo
Copy link
Member

tsalo commented Dec 28, 2019

No, thank you for opening the issue. I should have opened one to log the bug before fixing it in the linked PR.

@tsalo tsalo changed the title -ctab option Mismatch in -ctab option between json and tsv files Dec 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug issues describing a bug or error found in the project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants