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

[Module] Recognize_Bundles and Clean_Bundles #67

Merged
merged 11 commits into from
Apr 4, 2024

Conversation

CHrlS98
Copy link
Contributor

@CHrlS98 CHrlS98 commented Feb 8, 2024

Describe your changes

Add Recognize_Bundles and Clean_Bundles from rbx_flow.

Say which test data are used by your module

Uses scilpy bundles.zip test data.

A config.json file required for scil_recognize_multi_bundles.py is missing from the available test data.

Checklist before requesting a review

  • Create the tool:
    • Edit ./modules/nf-scil/<category>/<tool>/main.nf
    • Edit ./modules/nf-scil/<category>/<tool>/meta.yml
  • Generate the tests:
    • Edit ./tests/modules/nf-scil/<category>/<tool>/main.nf
    • Edit ./tests/modules/nf-scil/<category>/<tool>/nextflow.config
    • Add test data locally for tests with the fork repository
    • Generate the test infrastructure and md5sum for all outputs
  • Ensure the syntax is correct :
    • Check indentation abides with the rest of the library (don't hesitate to correct others !)
    • Lint everything. Ensure your variables have good names.

@AlexVCaron AlexVCaron mentioned this pull request Feb 22, 2024
@AlexVCaron AlexVCaron linked an issue Feb 22, 2024 that may be closed by this pull request
@AlexVCaron AlexVCaron removed a link to an issue Feb 27, 2024
@CHrlS98
Copy link
Contributor Author

CHrlS98 commented Feb 29, 2024

@AlexVCaron merged with main. I add some conflicts in file pytest_modules.yml I had to manually solve. Also, a reminder:

Uses scilpy bundles.zip test data.

A config.json file required for scil_recognize_multi_bundles.py is missing from the available test data.

@AlexVCaron AlexVCaron self-requested a review March 7, 2024 14:25
@AlexVCaron AlexVCaron changed the title Add Recognize_Bundles and Clean_Bundles [Module] Recognize_Bundles and Clean_Bundles Mar 7, 2024
@AlexVCaron AlexVCaron added this to the nf-scil modules v1.0 milestone Mar 9, 2024
@AlexVCaron
Copy link
Collaborator

Hi Charles ! You can update your PR with the main branch. Also, for the missing test data, can you prepare an archive or upload the missing file(s) here ? I'll make them available for your test cases.

@AlexVCaron
Copy link
Collaborator

Hi @CHrlS98 ! Can we try solving this PR ? There are two checks that fail, they should be fixable quickly :

  • Run prettier --write <module path>, it should fix the prettier check
  • Supply the data for the test, there are still some files missing I think

@gagnonanthony and @ThoumyreStanislas are available for support if you need a hand !

@CHrlS98
Copy link
Contributor Author

CHrlS98 commented Apr 2, 2024

@AlexVCaron @gagnonanthony @ThoumyreStanislas

Sorry for the delay it'd seem I missed the notification. I'm getting back to it this week.

@AlexVCaron AlexVCaron linked an issue Apr 2, 2024 that may be closed by this pull request
@CHrlS98
Copy link
Contributor Author

CHrlS98 commented Apr 2, 2024

config.json

@AlexVCaron @ThoumyreStanislas @gagnonanthony Here is the missing file for the tests. It should go in bundles.zip. This file is generated on the fly in the scilpy test (https://github.com/scilus/scilpy/blob/41078d0aa0aea4d71d5f5dcda514185754c53131/scripts/tests/test_tractogram_segment_bundles.py#L31).

@AlexVCaron AlexVCaron mentioned this pull request Apr 2, 2024
2 tasks
@CHrlS98
Copy link
Contributor Author

CHrlS98 commented Apr 3, 2024

I fixed the linting and the test input. However I did not run the test locally as it is unclear where I should put the config.json file. I may need some help with that.

@AlexVCaron
Copy link
Collaborator

Hi @CHrlS98 ! I am uploading the archive and updating the main right now, so you should be able to run the tests locally once you rebase or merge on those changes, I'll give you a heads up when ready. If you want to test with local files still, you can refer to #118.

@AlexVCaron
Copy link
Collaborator

@CHrlS98 The package is uploaded and main is up-to-date !

@CHrlS98
Copy link
Contributor Author

CHrlS98 commented Apr 3, 2024

Alright everything passes, ready for review @ThoumyreStanislas @gagnonanthony !

Copy link
Contributor

@gagnonanthony gagnonanthony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes, but overall really nice! Thanks @CHrlS98!

modules/nf-scil/bundle/recognize/main.nf Outdated Show resolved Hide resolved
modules/nf-scil/bundle/recognize/meta.yml Show resolved Hide resolved
modules/nf-scil/bundle/recognize/meta.yml Show resolved Hide resolved
tests/modules/nf-scil/bundle/recognize/nextflow.config Outdated Show resolved Hide resolved
Copy link
Contributor

@ThoumyreStanislas ThoumyreStanislas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's really good! Well done! Just a minor comment and that will be good for me.

file("${test_data_directory}/fibercup_atlas/")
]}

BUNDLE_RECOGNIZE ( input )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create another test in which you also test your parameters with other values (minimal_vote_ratio, seed, outlier_alpha).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in b331082 !

Copy link
Contributor

@gagnonanthony gagnonanthony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 💯

@ThoumyreStanislas
Copy link
Contributor

LGTM!

@AlexVCaron AlexVCaron merged commit 38c67c7 into scilus:main Apr 4, 2024
7 checks passed
@CHrlS98 CHrlS98 deleted the recognize_bundles branch April 4, 2024 17:14
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.

Hackathon PR merge
4 participants