Portability problems when running phantom example

[cncastillo]

Hi everyone,

I hope everything is going well. I have tried to run the example and had a few problems. I will describe the problems I encountered in "chronological order", trying to describe the experience of a new user trying to run this example.

To run the example in phantom data, I did not find the link to download the data within the joss_demo. For convenience, I would add a link there, so I don't need to return to the README. Preferably, I would not need to care about this; it should just download the data into data/ in the current directory.

The second problem I encountered was where to put recon_config_joss_demo.jl and data/ as it assumes a specific folder structure not described in the example (or at least not in the setup section). This could also be solved by downloading recon_config_joss_demo.jl into the current directory.

Finally I concluded that it needs to be like this:

data/
joss_demo.jl
recon_config_joss_demo.jl

Then, when I tried running it in a temp environment (clean env. like someone new to Julia), I realized that you are using mulitple packages:

using GIRFReco, MRIGradients
using MRIReco, FileIO, MRIFiles, MRIBase, MRICoilSensitivities
using RegularizedLeastSquares, Flux
using ImageTransformations
using PlotlyJS, Plots

Does someone that just want to run the example actually need to install all of them? When trying to install them, I had some disk space issues, but after deleting some files I managed to proceed. Instructions telling the user what packages to actually install (add PackageX) in the environment must be included, as GIRFReco has a lot of those inside now. This could be a simple Project.toml, as adding all of them manually is a little cumbersome.

Now, where I had some real problems is when it tried to run recon/cartesian_recon.jl that is also searching for "../src/utils/fieldmap_estimator.jl", similar to #9. This is a big portability problem of the example, as it assumes that I am running it within the repo. I am unsure where I need to run the example, maybe this assumes the folder structure before the JOSS changes (?). I could have downloaded the repo and run it there, but I believe this is an incorrect approach, and the user shouldn't need to do this. If the user needs additional functions to run an example, those need to be included in the package.

This is unfortunately as far as I got. I think the example needs to be refined, so the user can download joss_demo.jl and its Project.toml into a new folder, activate the Project.toml with ] activate . to install the necessary packages, run include("joss_demo.jl") in the REPL, and it should just work. These steps need to be described in the example with a warning telling the user that "X GB" will be downloaded for the phantom data. First impressions are very important for new users (especially new Julia users), and the provided example needs to be flawless.

Hope this is helpful, Cheers!

[mrikasper]

Dear Carlos @cncastillo,

Oh wow, sorry for all the issues when trying to run the example! I agree that this should have been a flawless experience for you (and any other user), and I am a bit confused why we didn't catch this earlier when going through all the feedback from the first round of reviews.

I thought we did quite some refactoring to have, for example, a specific Julia environment for the docs (and example), but maybe we are missing something fundamental here. Or could it be something as as simple as not having updated the README consistently?

For example, would a "Getting started" section like this work for the questions about the environments/packages to install?

using Pkg
Pkg.add("GIRFReco")

# activating example environment
cd docs
Pkg.activate(".")
Pkg.instantiate()

# run example
cd lit/examples
julia joss_demo.jl

One thing I am not sure about is how to navigate to the docs folder if users can be anywhere on their path.

Maybe the reason we did not spot this has to do with us having multiple (development) versions of the code and a specific IDE configuration on our machines instead of a clean slate? Just to double-check: You ran the current version of the code from the main branch?

Regarding the user experience, how would you go about making the example data more readily available? The original data is very large (as is raw MR data in general, unfortunately), but we could think about making only a subset of this available in a smaller ISMRMRD file. For downloading it automatically, how would you recommend implementing this in Julia? Via testing artifacts?

Again, so sorry for your difficulties, we will try to fix this soon, and any feedback to my questions here would be highly appreciated.

Thank you so much for your help!

All the best, Lars

[cncastillo]

Or could it be something as as simple as not having updated the README consistently?

I think it could be as simple as that, to update the instructions and add links to what needs to be downloaded. But that should be in https://brain-to.github.io/GIRFReco.jl/dev/generated/joss_demo/, not in the README. The example needs to be self-contained! There is no purpose to joss_demo, if I need to keep going back to the README to make the example work.

You ran the current version of the code from the main branch?

I am using the registered GIRFReco v0.1.1. I did not clone the repo or go to the package's location after the installation, as that is something a new user wouldn't do (the instructions say only to type ] add GIRFReco).

Regarding the user experience, how would you go about making the example data more readily available? The original data is very large (as is raw MR data in general, unfortunately), but we could think about making only a subset of this available in a smaller ISMRMRD file. For downloading it automatically, how would you recommend implementing this in Julia? Via testing artifacts?

I don't think the size of the data is really a problem, so I think it is okay to keep it as it is (with the corresponding warning in the demo). I am not completely sure about the artifacts. Just make sure that it is not downloaded when doing add GIRFReco. To download files programmatically, you can use Downloads.download. For the Zenodo data you could require doing it differently, but if the user is instructed in joss_demo to download the data (with a link) and unzip it following a certain folder structure, it should be fine.

I was imagining something like this at the beginning of joss_demo.jl:

# Getting necessary files
using Downloads
Downloads.download(example_zenodo_data_url, "./data.zip") #Then uncompress
Downloads.download(example_config_url, "./recon_config_joss_demo.jl")
Downloads.download(example_env_url, "./Project.toml")
# Unzip data
using ZipFile
unzip("./data.zip", exdir="./data")
# Setting environment
using Pkg
Pkg.activate(".") #Maybe something else is necessary here

I believe downloading data, a config file, Project.toml, and the actual example are reasonable, but not recon/ or the whole repo.

Now, where I had some real problems is when it tried to run recon/cartesian_recon.jl that is also searching for "../src/utils/fieldmap_estimator.jl", similar to https://github.com/BRAIN-TO/GIRFReco.jl/issues/9.

Another option, is to include files that are in the package with (do not cd to package):

include(joinpath(dirname(pathof(GIRFReco)), "../recon/cartesian_recon.jl"))

but it is putting the problem under the rug, if those functions are necessary to display the functionalities of the package, export them in the package or bundle them with the example. If you believe this is not the most basic example, you can simplify it. The user shouldn't need to run code inside the package.

[alexjaffray]

@cncastillo thanks for pointing these issues out and we can work towards solving them. The demo should be, as you said, self-contained.

I like your proposed solution using the Downloads and Zipfile approach to managing our external data location, we will proceed that way.

Regarding "./recon/cartesian_recon.jl", I will functionalize it properly, it makes sense to export it as a function instead of a script as suggested in #9.

[cncastillo]

Hi! Thanks for all the hard work. It is much easier to run the example now :smile: (include("run_example.jl")). I was able to replicate the results in the paper with ease.

My results: Results in the paper:

Some minor comments:

• If I delete data.zip, download_data.jl tries to download the data again, even if the data is already unzipped in joss_data_zenodo
• I was not able to set slice_choice = []; to anything else without having unexpected results (erros, or the reconstruction showed the wrong slices)
• The plots generated in Julia are a little stretched (not 1:1 aspect ratio). It was better to check the results in the produced NifTI files. On my NifTI viewer, scrolling through the slices looked weird (probably because of the diffusion dimension). I think a recommended viewer could be suggested.
• For convenience, I would paste part2 of the instructions in the example's README.md onto the beginning of the example, for the docs to be self-contained (someone could have missed the additional README).
• Maybe add some precomputed results in the docs? (like the one below without the GIRF corrections)

Congratulations again! You have addressed all my major comments :)

[nbwuzhe]

Hi @cncastillo , thanks (again) for your careful review of our code. This repo improved dramatically after taking the suggestions in this and your previous comments. Please accept my appreciation.

Regarding your comments:

Q1: If I delete data.zip, download_data.jl tries to download the data again, even if the data is already unzipped in joss_data_zenodo

A1: This has been fixed in the latest commits. Thanks for pointing out this issue.

Q2: I was not able to set slice_choice = []; to anything else without having unexpected results (erros, or the reconstruction showed the wrong slices)

A2: Really appreciate your careful test. We had a thorough test of multiple combinations of slice numbers & orders. This issue has been fixed in the latest commits.

Q3: The plots generated in Julia are a little stretched (not 1:1 aspect ratio). It was better to check the results in the produced NifTI files. On my NifTI viewer, scrolling through the slices looked weird (probably because of the diffusion dimension). I think a recommended viewer could be suggested.

A3: After surveying multiple backends of Plots, we chose PlotlyJS considering it's reaching a good balance between multiple factors. We have tried our best to achieve aspect ratio = 1 (we have specified this option in the heatmap call). As you are more experienced in Julia, do you have any further suggestions regarding image displaying (e.g. backend choice, configuration, etc.)

For now, we are only demonstrating our recon pipeline using b = 0 images, especially for multi-interleave recon (if this line is activated), so there may be some displaying issues across the dimension of diffusion for some NIFTI viewers. Meanwhile, our dataset has a relatively large slice gap (8mm gap vs 2mm slice thickness) so the slice location may not look smooth. We were majorly using FSLeyes and I have suggested it in the README.

Q4: For convenience, I would paste part2 of the instructions in the example's README.md onto the beginning of the example, for the docs to be self-contained (someone could have missed the additional README).

A4: This part is fulfilled. Thanks for the suggestion.

Q5: Maybe add some precomputed results in the docs? (like the one below without the GIRF corrections)

A5: We would like to give this opportunity to our readers/users to play around with the configurations of our example and see how the results change. I added a few words in the README.

[cncastillo]

A3: After surveying multiple backends of Plots, we chose PlotlyJS considering it's reaching a good balance between multiple factors. We have tried our best to achieve aspect ratio = 1 (we have specified this option in the heatmap call). As you are more experienced in Julia, do you have any further suggestions regarding image displaying (e.g. backend choice, configuration, etc.)

Looking at it again, the problem was not the aspect ratio of the original plot. The problem occurs when you zoom, as you can create any shape, distorting the image aspect ratio.

I know how to fix this with PlotlyJS.jl directly (yaxis=attr(scaleanchor="x", scaleratio=1)). However, as Plots.jl wraps PlotlyJS.jl, I am unsure how to modify it. Based on this:

https://stackoverflow.com/questions/69574843/zooming-in-does-not-adapt-date-axis-when-using-julias-plotlyjs-in-vscode

It seems that could be a limitation of Plots.jl, you can try adding ticks=:native. To be honest, now that there is a recommended NifTI viewer, that should be enough.

Congratulations again on the hard work! :smile:

[nbwuzhe]

A3: After surveying multiple backends of Plots, we chose PlotlyJS considering it's reaching a good balance between multiple factors. We have tried our best to achieve aspect ratio = 1 (we have specified this option in the heatmap call). As you are more experienced in Julia, do you have any further suggestions regarding image displaying (e.g. backend choice, configuration, etc.)

Looking at it again, the problem was not the aspect ratio of the original plot. The problem occurs when you zoom, as you can create any shape, distorting the image aspect ratio.

I know how to fix this with PlotlyJS.jl directly (yaxis=attr(scaleanchor="x", scaleratio=1)). However, as Plots.jl wraps PlotlyJS.jl, I am unsure how to modify it. Based on this:

https://stackoverflow.com/questions/69574843/zooming-in-does-not-adapt-date-axis-when-using-julias-plotlyjs-in-vscode

It seems that could be a limitation of Plots.jl, you can try adding ticks=:native. To be honest, now that there is a recommended NifTI viewer, that should be enough.

Congratulations again on the hard work! 😄

Hi @cncastillo really appreciate your insights.

I tried your suggestion using ticks=:native option. It has two issues with the current settings: (1) This option is in conflict with the yflip option we are using (they won't co-exist, complaining KeyError: key :range not found), which is quite essential to make sure the mosaic figure is displayed from top to bottom (the default will be from bottom to up, leaving upper right corner with blank subfigures when the columns of the last row are not filled). (2) Even if we removed yflip option, using ticks=:native will undo our efforts keeping the initial figure with aspect ratio = 1 (aspect_ratio option doesn't work anymore in this case). I will talk to @alexjaffray and @mrikasper to discuss whether it's a good idea to switch to native PlotlyJS to get rid of such a dilemma.

Kindly let us know if you have any further suggestions and we are happy to include them in our next release.

Best regards,

Tim

[alexjaffray]

@cncastillo @mrikasper @nbwuzhe the most recent push and release should fix everything but the plotting aspec ratio issue, and we now promote a NifTI viewer to see the results instead. Closing this.