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

Add work of Thomas Durantel #102

Merged
merged 14 commits into from
Nov 22, 2023
Merged

Add work of Thomas Durantel #102

merged 14 commits into from
Nov 22, 2023

Conversation

Florent2305
Copy link
Contributor

This PR contains part of Thomas Durantel's PhD work

  - Add a Track Orientation Distribution (TOD) image estimator (Dhollander et al. 2014).
Add some diffusion features developed by Thomas Durantel during his PHD
Copy link
Contributor

@astamm astamm left a comment

Choose a reason for hiding this comment

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

I have inserted a couple of comments and asked for changes. But maybe the best thing would be if I can work on the PR itself. Doable?

Anima/diffusion/odf/odf_average/CMakeLists.txt Outdated Show resolved Hide resolved
Anima/diffusion/odf/odf_average/animaODFAverage.cxx Outdated Show resolved Hide resolved
Anima/diffusion/odf/odf_average/animaODFAverage.cxx Outdated Show resolved Hide resolved
Anima/diffusion/odf/odf_average/animaODFAverage.cxx Outdated Show resolved Hide resolved
Anima/diffusion/odf/odf_average/animaODFAverage.cxx Outdated Show resolved Hide resolved
Anima/diffusion/odf/tod_estimator/animaTODEstimator.cxx Outdated Show resolved Hide resolved
Anima/diffusion/odf/tod_estimator/animaTODEstimator.cxx Outdated Show resolved Hide resolved
Anima/diffusion/odf/tod_estimator/animaTODEstimator.cxx Outdated Show resolved Hide resolved
@astamm
Copy link
Contributor

astamm commented Oct 4, 2023

If you check the box to allow edits from maintainers I'll be able to push to the PR branch on your fork to make a few fixes.

@astamm
Copy link
Contributor

astamm commented Oct 5, 2023

I see it is the case now. I will fork and work on it soon. Thanks @Florent2305!

@astamm
Copy link
Contributor

astamm commented Nov 22, 2023

I am done here. Thomas says it works as expected. @Florent2305 do you want to review or should I merge?

@astamm
Copy link
Contributor

astamm commented Nov 22, 2023

We can try to make a release of Anima before his defence now. That implies updating ITK and VTK and the documentation.

Copy link
Contributor Author

@Florent2305 Florent2305 left a comment

Choose a reason for hiding this comment

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

I made some comments. Can you answer it ?
After that it will be fine to merge.

"ODF images list as a text file",
true, "", "ODF images list", cmd);
"i", "input-odf-file",
"A string specifying the name of a text file in which the ODF images are listed.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A path or file name to specify the text file in which the ODF images are listed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

"m", "masks",
"Masks images list as a text file",
true, "", "Masks images list", cmd);
"w", "input-weight-file",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idem

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

"Result average ODF",
true, "", "result ODF image", cmd);
"o", "output-file",
"A string specifying the name of the output average ODF image.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idem

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

maskFiles.push_back(maskStr);
numInput++;
weightFiles.push_back(maskStr);
numInputs++;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

numInputs = weightFiles.size(); après l'accolade

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

std::vector<std::string> maskFiles;
unsigned int numInput = 0;
std::vector<std::string> weightFiles;
unsigned int numInputs = 0;
while (!odfFile.eof())
{
char tmpStr[2048], maskStr[2048];
odfFile.getline(tmpStr, 2048);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need error msg if line is over 2048

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

@astamm
Copy link
Contributor

astamm commented Nov 22, 2023

All fixed. We should be good to go. @Florent2305 let's just wait for the CI to complete. OK to merge after that?

@Florent2305
Copy link
Contributor Author

All fixed. We should be good to go. @Florent2305 let's just wait for the CI to complete. OK to merge after that?

Hey :D

@astamm astamm merged commit 7923db4 into Inria-Empenn:master Nov 22, 2023
1 check passed
@astamm astamm deleted the fusion branch November 22, 2023 18:10
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.

2 participants