Review Request: Henriques, Rokem, Garyfallidis, St-Jean, Perterson, Correia 1/2 #25

Merged
merged 7 commits into from Jan 5, 2017

Conversation

Projects
None yet
6 participants
@RafaelNH
Contributor

RafaelNH commented Dec 1, 2016

AUTHOR

Dear @ReScience/editors,

I request a review for the following replication:

Original article

Title: Optimization of a free water elimination two-compartment model for diffusion tensor imaging
Author(s): Andrew R. Hoy, Cheng G. Koay, Steven R. Kecskemeti, Andrew L. Alexander
Journal (or Conference): NeuroImage
Year: 2014
DOI: 10.1016/j.neuroimage.2014.09.053
PDF: http://ac.els-cdn.com/S1053811914007952/1-s2.0-S1053811914007952-main.pdf?_tid=7b211902-b7b7-11e6-be78-00000aab0f6b&acdnat=1480591115_87d58e852819d91f683039ddc22f47d3**

Replication

Author(s): Rafael Neto Henriques, Ariel Rokem, Eleftherios Garyfallidis, Samuel St-Jean, Eric Thomas Peterson, Marta Morgado Correia
Repository: https://github.com/RafaelNH/ReScience-submission/tree/RNH-AR-EG-SSTJ-ETP-MMC-2016
PDF: https://github.com/RafaelNH/ReScience-submission/blob/RNH-AR-EG-SSTJ-ETP-MMC-2016/article/RNH_AR_EG_SSTJ_ETP_MMC-2016.pdf
Keywords: Diffusion MRI; Diffusion modeling; Diffusion tensor imaging; Partial volume; cerebrospinal fluid; free water elimination; partial volume effect
Language: English
Domain: Life Science

Results

  • Article has been fully replicated
  • Article has been partially replicated
  • Article has not been replicated

Potential reviewers


EDITOR

  • Editor acknowledgment (@pdebuyl) 2 December 2016
  • Reviewer 1 (@delsuc) 5 December 2016
  • Reviewer 2 (@soolijoo) 18 December 2016
  • Review 1 decision [accept/reject]
  • Review 2 decision [accept/reject]
  • Editor decision [accept/reject]
@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Dec 1, 2016

Member

Thanks for your submission @RafaelNH. An editor will be assigned soon.

Member

rougier commented Dec 1, 2016

Thanks for your submission @RafaelNH. An editor will be assigned soon.

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Dec 2, 2016

Member

@pdebuyl Could you edit this submission ?

Member

rougier commented Dec 2, 2016

@pdebuyl Could you edit this submission ?

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Dec 2, 2016

Member

Hi @RafaelNH, @rougier,

I will be the editor for this submission. I am checking the repository and will look for reviewers.

Member

pdebuyl commented Dec 2, 2016

Hi @RafaelNH, @rougier,

I will be the editor for this submission. I am checking the repository and will look for reviewers.

@pdebuyl pdebuyl self-assigned this Dec 2, 2016

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Dec 2, 2016

Member

Hi @RafaelNH

Thank you for the submission to ReScience!

I have comments, before sending the submission for review:

  1. in code/LICENSE.txt, the copyright is to Rafael Neto Henriques and dipy developers. There should be the actual names of the authors of the files in code.
  2. there is no LICENSE file for the notebooks.
  3. The notebooks are saved as Python 2 but are actually Python 3 (as deduced from the use of the print function and the absence of from __future__ import ...
Member

pdebuyl commented Dec 2, 2016

Hi @RafaelNH

Thank you for the submission to ReScience!

I have comments, before sending the submission for review:

  1. in code/LICENSE.txt, the copyright is to Rafael Neto Henriques and dipy developers. There should be the actual names of the authors of the files in code.
  2. there is no LICENSE file for the notebooks.
  3. The notebooks are saved as Python 2 but are actually Python 3 (as deduced from the use of the print function and the absence of from __future__ import ...
@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Dec 5, 2016

Member

@delsuc can you review this?

Member

pdebuyl commented Dec 5, 2016

@delsuc can you review this?

@delsuc

This comment has been minimized.

Show comment
Hide comment
@delsuc

delsuc Dec 5, 2016

Hi @pdebuyl,
Yes I think I can review this work, the problem is that I am very busy for the moment, and I will not be able to work on this before mid-december (say dec 19th).

delsuc commented Dec 5, 2016

Hi @pdebuyl,
Yes I think I can review this work, the problem is that I am very busy for the moment, and I will not be able to work on this before mid-december (say dec 19th).

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Dec 5, 2016

Member

Hi @delsuc,

Thanks for the update. You will probably enter the game after the other reviewer (coming soon) but that is reasonable. (Unless I happen to find another more available reviewer by chance, in which case I would notify you immediately).

Member

pdebuyl commented Dec 5, 2016

Hi @delsuc,

Thanks for the update. You will probably enter the game after the other reviewer (coming soon) but that is reasonable. (Unless I happen to find another more available reviewer by chance, in which case I would notify you immediately).

@RafaelNH

This comment has been minimized.

Show comment
Hide comment
@RafaelNH

RafaelNH Dec 6, 2016

Contributor

Hi @pdebuyl
Thanks for your availability! I already addressed your three first comments. Let me known if something else is required at this point.

Contributor

RafaelNH commented Dec 6, 2016

Hi @pdebuyl
Thanks for your availability! I already addressed your three first comments. Let me known if something else is required at this point.

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Dec 6, 2016

Member

Hi @RafaelNH

Thanks for your updates. A first referee has accepted to review and should show up in this discussion soon. There is no action required from you at this point.

Regards,

Pierre

Member

pdebuyl commented Dec 6, 2016

Hi @RafaelNH

Thanks for your updates. A first referee has accepted to review and should show up in this discussion soon. There is no action required from you at this point.

Regards,

Pierre

@soolijoo

This comment has been minimized.

Show comment
Hide comment
@soolijoo

soolijoo Dec 9, 2016

Contributor

Hi @pdebuyl,

Happy to be on board -- nice to be part of an innovative review process!

Contributor

soolijoo commented Dec 9, 2016

Hi @pdebuyl,

Happy to be on board -- nice to be part of an innovative review process!

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Dec 9, 2016

Member

Hi Matt,

Once you are added to the reviewer team you will be able to tick the box "reviewer 1".

Member

pdebuyl commented Dec 9, 2016

Hi Matt,

Once you are added to the reviewer team you will be able to tick the box "reviewer 1".

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Dec 18, 2016

Member

@pdebuyl Any update on the second reviewer ?

Member

rougier commented Dec 18, 2016

@pdebuyl Any update on the second reviewer ?

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Dec 18, 2016

Member

Hi @rougier ,

The two reviewers are @delsuc and @soolijoo. I added the latter manually just now.

Member

pdebuyl commented Dec 18, 2016

Hi @rougier ,

The two reviewers are @delsuc and @soolijoo. I added the latter manually just now.

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Dec 18, 2016

Member

Oops sorry, didn't see it. Do not forget to update labels as well.

Member

rougier commented Dec 18, 2016

Oops sorry, didn't see it. Do not forget to update labels as well.

@pdebuyl pdebuyl added the 02 - Review label Dec 19, 2016

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
Member

pdebuyl commented Dec 21, 2016

@soolijoo @delsuc reminder

@delsuc

This comment has been minimized.

Show comment
Hide comment
@delsuc

delsuc Jan 4, 2017

General

This work implement and make available a method for the analysis of MRI images based on diffusion tensor. The original article presents a processing approach providing the required images, with details on the algorithm, but do not provides the code itself.

A new program is thus implemented which performs the same analysis. While different in the details, it reproduces the results found in the original work. This new implementation is written in python and open-source. It seems also to be significantly faster than the original implementation (but details are not available).

This work implmentens a slightly different minimisation algorithm, quoting the text :
"To improve computation speed, instead of using the modified Newton’s algorithm proposed in the original article, the non-linear convergence was done using Scipy’s wrapped modified Levenberg-Marquardt algorithm (function scipy.optimize.leastsq of Scipy)."
This is not a problem, as all correct optimisers should deliver the same result, (provided that they are fully converged) and differ only in the convergence speed. This approach also opens the possibility of implementing more advanced methods. The authors also test the newer scipy.optimize.least_square method, but to no avail.

Some parts of the original article are not reproduced. This is not a problem for the parts where the original paper explore the convergence properties of the algorithm (fig 1, 4, 6 of original paper).
This work does not reproduce the part where the original paper explores the best experimental set-up by simulating the results when varying the number of gradients and their intensities (fig 2 and 3).
These simulations are interesting as they allow one to tune before hand the measurement to a specific case, and reproducing these could be of help in other cases. The addition of this part to the work could be of great use, and would benefit the quality and impact of this work.

Text

The article is well written, and in some places, makes the original work clearer.
The details of implementation are well described.

remarks

  • Several typographical errors were found in the original paper and were rightfully corrected. (eq 3 and 5 from manuscript).
  • The vector $y$ and matrix $W$ (eq 6) have been re-ordered compared to the original work, while this is not per se a problem, the reason is not given.
  • In the case of pure free water, the basic fitted equation is ambiguous, leading to erratic results. The original work uses a special case for voxel where mean diffusivity values is larger than $1.5 \times 10 ^{-3} mm^{2}s^{-1}$, and this work uses $2.7 \times 10 ^{-3} mm^{2}s^{-1}$ as threshold, (knowing that $D_{iso} = 3 \times 10 ^{-3} mm^{2}s^{-1}$) there is no discussion for this change.

Code

The code ran directly from the notebook, using python 2.7,
however it does not run directly as downloaded from the repository, and a small modifictions was required to make the function directory accessible in the sys.path.
This should probably be mentionned/corrected.

The code makes intensive usage of the dipy module, I do not see this as a probleme, however this is unsufficiently documented:

  • Installation of the dipy program is required to run the code, this should be stressed, and installation of the dipy code should be described.
  • the use of dipy functions is made in the code without comment, and one has to go into the dipy source code itself to decipher twhat the functions realize.
    For instance in the run_sim.ipynb :
    • in the "Defining the acquisition parameters..." cell, the class HemiSphere is used, as well as some of its methods. This is used to generate a set shell directions.
    • in the next cell, a similar code is used to generate a set of random diffusion tensor directions for the MonteCarlo analysis.

Knowledge of dipy should not be assumed, and the usage of the different tools should be commented in the code.

misc remarks

  • in run_data in should be stressed that the line

    fetch_cenir_multib(with_raw=False)

downloads about 1.7 Gb of data in a hidden folder.

  • typo in supplementary_notebook_1.ipynb :
    "shows to be almost 4 times shower that the article's default procedure."
  • typo in supplementary_notebook_2.ipynb : "The Jacabian matrix"

Conclusion

This manuscript implementents in a succesful manner the original work, and provides a usable code.
I believe after the corrections mentionend above, it will be perfectly fitted for publication in the ReScience journal.

delsuc commented Jan 4, 2017

General

This work implement and make available a method for the analysis of MRI images based on diffusion tensor. The original article presents a processing approach providing the required images, with details on the algorithm, but do not provides the code itself.

A new program is thus implemented which performs the same analysis. While different in the details, it reproduces the results found in the original work. This new implementation is written in python and open-source. It seems also to be significantly faster than the original implementation (but details are not available).

This work implmentens a slightly different minimisation algorithm, quoting the text :
"To improve computation speed, instead of using the modified Newton’s algorithm proposed in the original article, the non-linear convergence was done using Scipy’s wrapped modified Levenberg-Marquardt algorithm (function scipy.optimize.leastsq of Scipy)."
This is not a problem, as all correct optimisers should deliver the same result, (provided that they are fully converged) and differ only in the convergence speed. This approach also opens the possibility of implementing more advanced methods. The authors also test the newer scipy.optimize.least_square method, but to no avail.

Some parts of the original article are not reproduced. This is not a problem for the parts where the original paper explore the convergence properties of the algorithm (fig 1, 4, 6 of original paper).
This work does not reproduce the part where the original paper explores the best experimental set-up by simulating the results when varying the number of gradients and their intensities (fig 2 and 3).
These simulations are interesting as they allow one to tune before hand the measurement to a specific case, and reproducing these could be of help in other cases. The addition of this part to the work could be of great use, and would benefit the quality and impact of this work.

Text

The article is well written, and in some places, makes the original work clearer.
The details of implementation are well described.

remarks

  • Several typographical errors were found in the original paper and were rightfully corrected. (eq 3 and 5 from manuscript).
  • The vector $y$ and matrix $W$ (eq 6) have been re-ordered compared to the original work, while this is not per se a problem, the reason is not given.
  • In the case of pure free water, the basic fitted equation is ambiguous, leading to erratic results. The original work uses a special case for voxel where mean diffusivity values is larger than $1.5 \times 10 ^{-3} mm^{2}s^{-1}$, and this work uses $2.7 \times 10 ^{-3} mm^{2}s^{-1}$ as threshold, (knowing that $D_{iso} = 3 \times 10 ^{-3} mm^{2}s^{-1}$) there is no discussion for this change.

Code

The code ran directly from the notebook, using python 2.7,
however it does not run directly as downloaded from the repository, and a small modifictions was required to make the function directory accessible in the sys.path.
This should probably be mentionned/corrected.

The code makes intensive usage of the dipy module, I do not see this as a probleme, however this is unsufficiently documented:

  • Installation of the dipy program is required to run the code, this should be stressed, and installation of the dipy code should be described.
  • the use of dipy functions is made in the code without comment, and one has to go into the dipy source code itself to decipher twhat the functions realize.
    For instance in the run_sim.ipynb :
    • in the "Defining the acquisition parameters..." cell, the class HemiSphere is used, as well as some of its methods. This is used to generate a set shell directions.
    • in the next cell, a similar code is used to generate a set of random diffusion tensor directions for the MonteCarlo analysis.

Knowledge of dipy should not be assumed, and the usage of the different tools should be commented in the code.

misc remarks

  • in run_data in should be stressed that the line

    fetch_cenir_multib(with_raw=False)

downloads about 1.7 Gb of data in a hidden folder.

  • typo in supplementary_notebook_1.ipynb :
    "shows to be almost 4 times shower that the article's default procedure."
  • typo in supplementary_notebook_2.ipynb : "The Jacabian matrix"

Conclusion

This manuscript implementents in a succesful manner the original work, and provides a usable code.
I believe after the corrections mentionend above, it will be perfectly fitted for publication in the ReScience journal.

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Jan 4, 2017

Member

Hi @delsuc thank you for the review!
@RafaelNH you can wait for the review by @soolijoo before proceeding or, if you wish, already work on an update. If so, please mention it for @soolijoo to avoid reviewing the paper and its update if not necessary.

Member

pdebuyl commented Jan 4, 2017

Hi @delsuc thank you for the review!
@RafaelNH you can wait for the review by @soolijoo before proceeding or, if you wish, already work on an update. If so, please mention it for @soolijoo to avoid reviewing the paper and its update if not necessary.

@soolijoo soolijoo merged commit 163cf25 into ReScience:master Jan 5, 2017

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Jan 5, 2017

Member

Hi @soolijoo, you just merged the paper by Henriques et al in the main submission repository of ReScience. The purpose of "ReScience-submission" is only to do the reviewing process. Let me know if you need any assistance with the git/GitHub system.

@rougier I have a "revert" button at my disposal, it is tempting to use it but I prefer to check with you before proceeding.

Member

pdebuyl commented Jan 5, 2017

Hi @soolijoo, you just merged the paper by Henriques et al in the main submission repository of ReScience. The purpose of "ReScience-submission" is only to do the reviewing process. Let me know if you need any assistance with the git/GitHub system.

@rougier I have a "revert" button at my disposal, it is tempting to use it but I prefer to check with you before proceeding.

@soolijoo

This comment has been minimized.

Show comment
Hide comment
@soolijoo

soolijoo Jan 5, 2017

Contributor

Hi @pdebuyl,

That would be helpful. can you let me know what I was supposed to do?

Contributor

soolijoo commented Jan 5, 2017

Hi @pdebuyl,

That would be helpful. can you let me know what I was supposed to do?

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Jan 5, 2017

Member

@rougier @khinsen should we/I do a plain revert (that will show in the history) or force-push a git reset --hard ?

Member

pdebuyl commented Jan 5, 2017

@rougier @khinsen should we/I do a plain revert (that will show in the history) or force-push a git reset --hard ?

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Jan 5, 2017

Member

Maybe a hard reset would be better, we don't want to pollute future submission.
I didn't know reviewer were granted merge privileges. I'll try to fix that to avoid future accidents.

Member

rougier commented Jan 5, 2017

Maybe a hard reset would be better, we don't want to pollute future submission.
I didn't know reviewer were granted merge privileges. I'll try to fix that to avoid future accidents.

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Jan 5, 2017

Member

@soolijoo No problem, we'll fix the error. For posting your review, you just have to comment on this thread like @delsuc did.

Member

rougier commented Jan 5, 2017

@soolijoo No problem, we'll fix the error. For posting your review, you just have to comment on this thread like @delsuc did.

@soolijoo

This comment has been minimized.

Show comment
Hide comment
@soolijoo

soolijoo Jan 5, 2017

Contributor

General

This submission is a new implementation of previous approach for the elimination of free water fraction from diffusion-weighted data. It makes a number of changes to the original methodology designed to improve performance and robustness.

The code is tested using synthetic and in vivo data and is provided in python, and apparently improves significantly on the original implementation.

The synthetic data is from the same model as that fitted with an additional noise component. This is not a particularly stringent test of the approach, although it does provide a decent sanity-check. The in vivo data is a stronger check, but provides no ground truth so. What is being gained from this approach, and why is it worth using?

Text

The initial motivation for this technique is a little unclear. If I am concerned about free-water in my data, wouldn't I just null it out in the acquisition? Furthermore, the approach requires multi-shell data and therefore would not, in general, enable re-analysis of old data, which is typically acquired on a single shell.

Page 1, paragraph 2: "For example FA is thought to be an indicator of..." -- This statement is misleading and should be reworded. FA is a measure of the overall shape of the fitted tensor. It is influenced by axons geometry, density, cogerence, etc, but it is certainly not an indicator of any one of them. The measure cannot be inverted unambiguously, and is a summary statistic only. Please change this sentence.

The authors state that they have chosen a python implementation and then motivate the new implementation largely on the grounds of speed. Python is hardly the most efficient way to implement a numerical technique, which makes these statements a little contradictory. If you are interested in efficiency, why choose python? Given the simplicity of the model and fitting proposed, I would expect an efficent C++ implementation on a typical desktop PC (as mentioed inthe text) to run in seconds. Instead this code takes upwards of 20 minutes on my machine. Robustness, yes. Ease of implementation, yes. Speed? definitely not.

The synthetic data is described as being from Monte-Carlo simulation, but this is not correct. The authors synthesise data using a model and add noise. Monte-Carlo simulation involves using random numbers to approximate integrals that are otherwise intractable analytically. Nothing of that sort is going on here. Please amend the terminology.

The procedure detailed on page 3 to address the degeneracy in the model when only the free compartment is present is quite ad-hoc. Clearly this is necessary, but there is little discussion of how the parameter were chosen. There is also no discussion of alternative approaches or how rfrequntly this problem arises. It's a bit hacky.

Page 3/4 "theoretical free diffusion value" -- please clarify where this number comes from? If it is a value from the literature it needs a citation. I strongly suspect that it is an experimental measurement, not a theoretical calculation.

The SNR in the simulation experiments (40) seems high. 20 is more typical in diffusion MRI experiments, and no discussion is given of how this value was chosen.

Why 120 directions and 100 repeats?

Page 5, para 1: "FA values... match the tissue's ground truth". What do you mean by "match"? please be more specific and make a quantitative statement.

Code

The code ran as downloaded using python 2.7. No alterations were necessary. Figures were reproduced as in the main text. Synthetic data script ran in 23 mins, in vivo data in 34 minutes on a Dell Precision T7610 with dual 8 core 2.4GHz Xeons and 128Gb RAM.

Looking a little deeper, sphere point picking is not performed correctly, meaning that the 120 directions used will be biased around the poles of the coordinate system. This is simple to fix, see the following for details:
http://mathworld.wolfram.com/SpherePointPicking.html

Conclusion

The code runs and produces the results as reported in the paper. The top-level of the code is cleqar enough, although the use of multiple libraries means that I have to trust that the dependencies are working without issue. I am a little unclear as to why I should use this approach, however.

Minor points

page 3, text beneath eq.7 -- typo in the LaTeX. D_tissue should be D_{tissue}
page 3, final line -- arbitray -> arbitrary
The constrast in fig-2 could be improved.

Contributor

soolijoo commented Jan 5, 2017

General

This submission is a new implementation of previous approach for the elimination of free water fraction from diffusion-weighted data. It makes a number of changes to the original methodology designed to improve performance and robustness.

The code is tested using synthetic and in vivo data and is provided in python, and apparently improves significantly on the original implementation.

The synthetic data is from the same model as that fitted with an additional noise component. This is not a particularly stringent test of the approach, although it does provide a decent sanity-check. The in vivo data is a stronger check, but provides no ground truth so. What is being gained from this approach, and why is it worth using?

Text

The initial motivation for this technique is a little unclear. If I am concerned about free-water in my data, wouldn't I just null it out in the acquisition? Furthermore, the approach requires multi-shell data and therefore would not, in general, enable re-analysis of old data, which is typically acquired on a single shell.

Page 1, paragraph 2: "For example FA is thought to be an indicator of..." -- This statement is misleading and should be reworded. FA is a measure of the overall shape of the fitted tensor. It is influenced by axons geometry, density, cogerence, etc, but it is certainly not an indicator of any one of them. The measure cannot be inverted unambiguously, and is a summary statistic only. Please change this sentence.

The authors state that they have chosen a python implementation and then motivate the new implementation largely on the grounds of speed. Python is hardly the most efficient way to implement a numerical technique, which makes these statements a little contradictory. If you are interested in efficiency, why choose python? Given the simplicity of the model and fitting proposed, I would expect an efficent C++ implementation on a typical desktop PC (as mentioed inthe text) to run in seconds. Instead this code takes upwards of 20 minutes on my machine. Robustness, yes. Ease of implementation, yes. Speed? definitely not.

The synthetic data is described as being from Monte-Carlo simulation, but this is not correct. The authors synthesise data using a model and add noise. Monte-Carlo simulation involves using random numbers to approximate integrals that are otherwise intractable analytically. Nothing of that sort is going on here. Please amend the terminology.

The procedure detailed on page 3 to address the degeneracy in the model when only the free compartment is present is quite ad-hoc. Clearly this is necessary, but there is little discussion of how the parameter were chosen. There is also no discussion of alternative approaches or how rfrequntly this problem arises. It's a bit hacky.

Page 3/4 "theoretical free diffusion value" -- please clarify where this number comes from? If it is a value from the literature it needs a citation. I strongly suspect that it is an experimental measurement, not a theoretical calculation.

The SNR in the simulation experiments (40) seems high. 20 is more typical in diffusion MRI experiments, and no discussion is given of how this value was chosen.

Why 120 directions and 100 repeats?

Page 5, para 1: "FA values... match the tissue's ground truth". What do you mean by "match"? please be more specific and make a quantitative statement.

Code

The code ran as downloaded using python 2.7. No alterations were necessary. Figures were reproduced as in the main text. Synthetic data script ran in 23 mins, in vivo data in 34 minutes on a Dell Precision T7610 with dual 8 core 2.4GHz Xeons and 128Gb RAM.

Looking a little deeper, sphere point picking is not performed correctly, meaning that the 120 directions used will be biased around the poles of the coordinate system. This is simple to fix, see the following for details:
http://mathworld.wolfram.com/SpherePointPicking.html

Conclusion

The code runs and produces the results as reported in the paper. The top-level of the code is cleqar enough, although the use of multiple libraries means that I have to trust that the dependencies are working without issue. I am a little unclear as to why I should use this approach, however.

Minor points

page 3, text beneath eq.7 -- typo in the LaTeX. D_tissue should be D_{tissue}
page 3, final line -- arbitray -> arbitrary
The constrast in fig-2 could be improved.

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Jan 5, 2017

Member

@rougier a git reset --hard 4efcb19a0593d1c7a835644daa47224c27476b9a followed by git push --force should do. I can proceed if you wish but wait for your instructions anyway to

Member

pdebuyl commented Jan 5, 2017

@rougier a git reset --hard 4efcb19a0593d1c7a835644daa47224c27476b9a followed by git push --force should do. I can proceed if you wish but wait for your instructions anyway to

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Jan 5, 2017

Member

@soolijoo Thank you for the review!

In your report, you write

It makes a number of changes to the original methodology

Does this comment refer to changes made by Hoy et al with respect to earlier approaches or to changes made by Henriques et al with respect to Hoy et al?

@RafaelNH please wait for us to have fixed the closing of the PR before updating the submission (code and/or paper). You can use this discussion page anyway for the replies.

Member

pdebuyl commented Jan 5, 2017

@soolijoo Thank you for the review!

In your report, you write

It makes a number of changes to the original methodology

Does this comment refer to changes made by Hoy et al with respect to earlier approaches or to changes made by Henriques et al with respect to Hoy et al?

@RafaelNH please wait for us to have fixed the closing of the PR before updating the submission (code and/or paper). You can use this discussion page anyway for the replies.

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Jan 5, 2017

Member

@pdebuyl Ok, you can proceed.

Member

rougier commented Jan 5, 2017

@pdebuyl Ok, you can proceed.

@soolijoo

This comment has been minimized.

Show comment
Hide comment
@soolijoo

soolijoo Jan 5, 2017

Contributor

@pdebuyl to the original Hoy paper. The changes they make are pretty standard with respect to other existing literature.

Contributor

soolijoo commented Jan 5, 2017

@pdebuyl to the original Hoy paper. The changes they make are pretty standard with respect to other existing literature.

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Jan 5, 2017

Member

@rougier Done.

@soolijoo Thank you for the precision.

@RafaelNH You can now update your repository. I reset the master branch here but that did not automatically re-open the pull request. We will proceed with the current pull request even though it is marked as closed.

Member

pdebuyl commented Jan 5, 2017

@rougier Done.

@soolijoo Thank you for the precision.

@RafaelNH You can now update your repository. I reset the master branch here but that did not automatically re-open the pull request. We will proceed with the current pull request even though it is marked as closed.

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Jan 11, 2017

Member

@RafaelNH have you seen this update?

Member

pdebuyl commented Jan 11, 2017

@RafaelNH have you seen this update?

@RafaelNH

This comment has been minimized.

Show comment
Hide comment
@RafaelNH

RafaelNH Jan 11, 2017

Contributor

Hi @pdebuyl! Thanks for your update and sorting the PR's early merging issue! I've already started working on the changes. I will submit some comments asap!

Contributor

RafaelNH commented Jan 11, 2017

Hi @pdebuyl! Thanks for your update and sorting the PR's early merging issue! I've already started working on the changes. I will submit some comments asap!

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Jan 11, 2017

Member

Hi, thanks for the feedback. No intention to push you, I just wanted to check that you knew about the reviews.

Member

pdebuyl commented Jan 11, 2017

Hi, thanks for the feedback. No intention to push you, I just wanted to check that you knew about the reviews.

@RafaelNH

This comment has been minimized.

Show comment
Hide comment
@RafaelNH

RafaelNH Feb 7, 2017

Contributor

Hi @pdebuyl,
I was not able to update this PR because it was merged. I decided to create PR #26 to address the comments made by @delsuc and @soolijoo.

Contributor

RafaelNH commented Feb 7, 2017

Hi @pdebuyl,
I was not able to update this PR because it was merged. I decided to create PR #26 to address the comments made by @delsuc and @soolijoo.

@arokem

This comment has been minimized.

Show comment
Hide comment
@arokem

arokem Feb 7, 2017

Hey @pdebuyl : would you mind if we uploaded a version of this (rendered with the ReScience pdf format) to arXiv/biorXiv? We need this for an upcoming deadline.

arokem commented Feb 7, 2017

Hey @pdebuyl : would you mind if we uploaded a version of this (rendered with the ReScience pdf format) to arXiv/biorXiv? We need this for an upcoming deadline.

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Feb 7, 2017

Member

Hi @RafaelNH

If I understand correctly it is indeed impossible to update closed PR (apart from discussing as we do now). I'll just wait a green light from @rougier or @khinsen to follow the submission in a second PR.

@arokem No issue here. Your work is CC-BY. I would make sure to update the DOI and Journal Ref when the paper is publisehd though :-)

Member

pdebuyl commented Feb 7, 2017

Hi @RafaelNH

If I understand correctly it is indeed impossible to update closed PR (apart from discussing as we do now). I'll just wait a green light from @rougier or @khinsen to follow the submission in a second PR.

@arokem No issue here. Your work is CC-BY. I would make sure to update the DOI and Journal Ref when the paper is publisehd though :-)

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Feb 7, 2017

Member

I'm a bit lost on the whole process now...

@pdebuyl For me it seems ok to continue on the new PR maybe with some context at the top and a link to this PR, as well as a link to the new PR at the end of this one. Also, maybe you can add 1/2 to the title of this PR and 2/2 to the title of second one. And don't forget to update all the different labels. Last, you can lock the conversation here to prevent any further comment.

@arokem Yep, you're free to upload the paper wherever you want but you may need to put "under review" somewhere since it is not yet officially accepted/published. And be careful with the buttons on the left, they might not be relevant yet.

Member

rougier commented Feb 7, 2017

I'm a bit lost on the whole process now...

@pdebuyl For me it seems ok to continue on the new PR maybe with some context at the top and a link to this PR, as well as a link to the new PR at the end of this one. Also, maybe you can add 1/2 to the title of this PR and 2/2 to the title of second one. And don't forget to update all the different labels. Last, you can lock the conversation here to prevent any further comment.

@arokem Yep, you're free to upload the paper wherever you want but you may need to put "under review" somewhere since it is not yet officially accepted/published. And be careful with the buttons on the left, they might not be relevant yet.

@pdebuyl pdebuyl changed the title from Review Request: Henriques, Rokem, Garyfallidis, St-Jean, Perterson, Correia to Review Request: Henriques, Rokem, Garyfallidis, St-Jean, Perterson, Correia 1/2 Feb 7, 2017

@ReScience ReScience locked and limited conversation to collaborators Feb 7, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.