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

[WIP] Fibertube Tracking #971

Open
wants to merge 64 commits into
base: master
Choose a base branch
from
Open

Conversation

VincentBeaud
Copy link
Contributor

@VincentBeaud VincentBeaud commented Apr 16, 2024

Quick description

Please include a summary of the changes and the related issue(s) or improvement(s).
Please also include relevant motivation and context. List any dependencies that are required for this change if needed.

...

Type of change

Check the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Provide data, screenshots, command line to test (if relevant)

...

Checklist

  • My code follows the style guidelines of this project (run autopep8)
  • I added relevant citations to scripts, modules and functions docstrings and descriptions
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I moved all functions from the script file (except the argparser and main) to scilpy modules
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@pep8speaks
Copy link

pep8speaks commented Apr 16, 2024

Hello @VincentBeaud, Thank you for updating !

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

Comment last updated at 2024-10-25 21:03:20 UTC

@VincentBeaud
Copy link
Contributor Author

VincentBeaud commented Apr 16, 2024

@mdesco et @CHrlS98 Voici la grosse PR! Il me reste un peu de job à faire alors elle est en draft. Aussi, je viens de faire plusieurs petits changements et je n'ai pas retesté chaque script. Je vais donc le faire maintenant en créant un fichier de test pour chaque.

Je vais aussi rajouter une description détaillée dès que j'aurai terminé ça.

Copy link

codecov bot commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 59.42184% with 379 lines in your changes missing coverage. Please review.

Project coverage is 68.50%. Comparing base (1d6f848) to head (c846a8c).
Report is 36 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #971      +/-   ##
==========================================
- Coverage   68.78%   68.50%   -0.28%     
==========================================
  Files         429      439      +10     
  Lines       22265    23388    +1123     
  Branches     3324     3174     -150     
==========================================
+ Hits        15314    16022     +708     
- Misses       5665     6040     +375     
- Partials     1286     1326      +40     
Components Coverage Δ
Scripts 69.85% <77.81%> (+0.19%) ⬆️
Library 66.67% <49.83%> (-0.87%) ⬇️

Copy link
Contributor

@mdesco mdesco left a comment

Choose a reason for hiding this comment

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

First 2 files of the PR tested and commented. Great job! Great start.

Major comments to discussion with @CHrlS98 and maybe @arnaudbore

  1. Should we rename all files that contain ft or fibertube. If not, how do we make sure our users know what ft means?

  2. @VincentBeaud I think you should create a small documentation in docs/source/documentation/ to show the typical order of execution of your scripts.

  • scil_tractogram_filter_collisions.py GT_tracks.trk diameter.txt GT_tracks_wo_collisions.trk --single_diameter --save_colliders --save_collided
  • scil_ft_visualize_collisions.py GT_tracks_wo_collisions_colliders.trk
  • ...

And maybe ask @arnaudbore where you could put a small toy exemple of trk and diameters.

scripts/scil_tractogram_filter_collisions.py Outdated Show resolved Hide resolved
scripts/scil_tractogram_filter_collisions.py Show resolved Hide resolved
scripts/scil_tractogram_filter_collisions.py Outdated Show resolved Hide resolved
scripts/scil_tractogram_filter_collisions.py Outdated Show resolved Hide resolved
scripts/scil_tractogram_filter_collisions.py Outdated Show resolved Hide resolved
scripts/scil_ft_visualize_collisions.py Outdated Show resolved Hide resolved
scripts/scil_ft_visualize_collisions.py Outdated Show resolved Hide resolved
scripts/scil_ft_visualize_collisions.py Outdated Show resolved Hide resolved
scripts/scil_ft_visualize_collisions.py Outdated Show resolved Hide resolved
scripts/scil_ft_visualize_collisions.py Outdated Show resolved Hide resolved
@CHrlS98
Copy link
Contributor

CHrlS98 commented Apr 17, 2024

@mdesco

  1. Should we rename all files that contain ft or fibertube. If not, how do we make sure our users know what ft means?

Yes I think we should replace ft by fibertube. We will want our example in the documentation to clearly explain what fibertubes are too.

Copy link
Contributor

@CHrlS98 CHrlS98 left a comment

Choose a reason for hiding this comment

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

Reviewed everything but did not test anything. This is a big PR. Ask me if you have any questions. My comments are mostly about scilpy coding standards and some renaming suggestions. Good work!

scripts/scil_ft_fibers_metrics.py Outdated Show resolved Hide resolved
scripts/scil_ft_fibers_metrics.py Outdated Show resolved Hide resolved
scripts/scil_ft_fibers_metrics.py Outdated Show resolved Hide resolved
scripts/scil_ft_fibers_metrics.py Outdated Show resolved Hide resolved
scripts/scil_ft_fibers_metrics.py Outdated Show resolved Hide resolved
scripts/scil_ft_reconstruction_metrics.py Outdated Show resolved Hide resolved
scripts/tests/test_ft_tracking.py Outdated Show resolved Hide resolved
scripts/tests/test_ft_reconstruction_metrics.py Outdated Show resolved Hide resolved
scripts/scil_ft_fibers_metrics.py Outdated Show resolved Hide resolved
scripts/scil_ft_reconstruction_metrics.py Outdated Show resolved Hide resolved
@VincentBeaud
Copy link
Contributor Author

VincentBeaud commented Apr 19, 2024

New script names:
scil_tractogram_filter_collisions.py -> unchanged
scil_ft_visualize_collisions.py -> scil_viz_tractogram_collisions.py
scil_ft_fibers_metrics.py -> deleted. Code is in "scil_tractogram_filter_collisions.py"
scil_ft_tracking.py -> scil_fibertube_tracking.py
scil_ft_reconstruction_metrics.py -> scil_fibertube_score_tractogram.py

@arnaudbore arnaudbore changed the title Fibertube Tracking [WIP] Fibertube Tracking Jun 18, 2024
Copy link
Contributor

@mdesco mdesco left a comment

Choose a reason for hiding this comment

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

Good job @VincentBeaud . Massive PR and lots of nice work.

  1. Good news. Everything runs well.
  2. My comments are mostly regarding clarity of the docstring and --help messages. I'm picky on terminology and coherence across scripts. Addressing my comments will help your paper and demo

scripts/scil_ft_tracking.py Outdated Show resolved Hide resolved
scripts/scil_ft_tracking.py Outdated Show resolved Hide resolved
scripts/scil_ft_tracking.py Outdated Show resolved Hide resolved
scripts/scil_ft_tracking.py Outdated Show resolved Hide resolved
scripts/scil_ft_tracking.py Outdated Show resolved Hide resolved
scripts/scil_fibertube_score_tractogram.py Show resolved Hide resolved
scripts/scil_fibertube_score_tractogram.py Outdated Show resolved Hide resolved
scripts/scil_fibertube_tracking.py Show resolved Hide resolved
scripts/scil_fibertube_score_tractogram.py Outdated Show resolved Hide resolved
- me_min
- me_max
- me_mean
- me_med
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you and could you propose a figure to illustrate these? A bit like you did above?

docs/fibertube/DEMO.md Outdated Show resolved Hide resolved
docs/fibertube/DEMO.md Show resolved Hide resolved
docs/fibertube/DEMO.md Outdated Show resolved Hide resolved
docs/fibertube/DEMO.md Outdated Show resolved Hide resolved
docs/fibertube/DEMO.md Show resolved Hide resolved
docs/fibertube/DEMO.md Outdated Show resolved Hide resolved
docs/fibertube/DEMO.md Show resolved Hide resolved
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.

5 participants