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

Save manually classified components into txt files #58

Merged
merged 6 commits into from
Apr 28, 2023

Conversation

eurunuela
Copy link
Collaborator

Closes #57.

This PR adds the following new functionality to Rica:

  • Manually accepted components are saved into a .txt file and their indices are comma-separated.
  • Manually rejected components are saved into a .txt file and their indices are comma-separated.
  • The manual_classification.tsv file is still saved for backward compatibility.

@netlify
Copy link

netlify bot commented Jan 17, 2023

Deploy Preview for rica-fmri ready!

Name Link
🔨 Latest commit 148d444
🔍 Latest deploy log https://app.netlify.com/sites/rica-fmri/deploys/644beba3428d750008ab64be
😎 Deploy Preview https://deploy-preview-58--rica-fmri.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@eurunuela
Copy link
Collaborator Author

@jbteves @handwerkerd could you please test using the following link?

https://deploy-preview-58--rica-fmri.netlify.app/

@handwerkerd
Copy link
Member

I just made some changes to ica_reclassify in https://github.com/jbteves/tedana/pull/42 and then this PR works well.

There is one minor issue I saw. Rica write out manual_classification.tsv. The file includes the I0XX classification identifiers that were removed from the DTM branch and replaced with descriptors in a classification_tags column. This isn't great because Rica is adding those I0XX values to a new column without a header.
For the DTM branch, classification_tags can include a comma separated string of elements so you could add ,manual reclassify to that string. This is fairly minor because ica_reclassify doesn't use your manual_classification.tsv and will correctly write out an internally consistent component table when run.

@eurunuela
Copy link
Collaborator Author

Just pushed the changes.

Now Rica will add , Manual reclassify with Rica to the classification_tags column.

What do you think @handwerkerd?

@handwerkerd
Copy link
Member

I'm getting an error message:

$ npm install
$ npm start

Failed to compile.

Module not found: Error: Can't resolve 'react-helmet' in '/Users/handwerkerd/code/meica/rica/src'
WARNING in [eslint] 
src/Plots/Plots.js
  Line 225:14:  'i' is already defined                             no-redeclare
  Line 367:15:  'datasetIndex' is assigned a value but never used  no-unused-vars
  Line 381:15:  'datasetIndex' is assigned a value but never used  no-unused-vars

ERROR in ./src/index.js 6:0-38
Module not found: Error: Can't resolve 'react-helmet' in '/Users/handwerkerd/code/meica/rica/src'

webpack compiled with 1 error and 1 warning

I feel like I saw this before when I forgot to run npm install first, but not sure what's going on now.

@eurunuela
Copy link
Collaborator Author

It looks like I removed a dependency when fixing the merge conflict. Sorry about that.

Could you try now, please?

In any case, you don't need to try to build the project locally. You can try the deploy preview link given by the netlify bot at the top of this conversation.

@handwerkerd
Copy link
Member

I just tried again. Locally, npm install; npm start still gives the same here. The deploy preview link did run. The classification_tag is filled correctly, but, when I saved the file, it's no longer changing the component classifications in any of the 3 files.

@eurunuela
Copy link
Collaborator Author

Sorry, my bad. I forgot I was using the rationale for the accepted and rejected files.

The last commit should have fixed this. Could you please try again @handwerkerd?

@handwerkerd
Copy link
Member

It's all working now. I needed to delete the node_modules directory and (maybe) package-lock.json to have npm install; npm start work again, but that also works now. This should be ready to merge.

@eurunuela
Copy link
Collaborator Author

Thank you Dan!

@eurunuela eurunuela merged commit eb01154 into ME-ICA:master Apr 28, 2023
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.

Get RICA to work with tedana_reclassify
2 participants