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

Convert to NeuroLibre submission #91

Merged
merged 27 commits into from
Feb 14, 2024
Merged

Convert to NeuroLibre submission #91

merged 27 commits into from
Feb 14, 2024

Conversation

mathieuboudreau
Copy link
Member

@mathieuboudreau mathieuboudreau commented Feb 8, 2024

Resolves #42

Live link to Jupyter Book: https://shimming-toolbox.github.io/rf-shimming-7t/

Currently all the working links point to this branch for the review so that they're usable and viewable, will need to change all instances of mb/neurolibre to main prior to merging.

[![Open In Colab](https://colab.research.google.com/assets/colab-badge.svg)](https://colab.research.google.com/github/shimming-toolbox/rf-shimming-7t/blob/mb/neurolibre/content/index.ipynb)
[![launch binder](https://mybinder.org/badge_logo.svg)](https://mybinder.org/v2/gh/shimming-toolbox/rf-shimming-7t/mb/neurolibre?labpath=content%2Findex.ipynb)

To launch a Docker session from this repo, run `repo2docker --ref mb/neurolibre https://www.github.com/shimming-toolbox/rf-shimming-7t`. After it's completed, it will provie you with a weblink, copy and open this link in a browser to open the Jupyter Notebook session.

git checkout mb/neurolibre


branch: mb/neurolibre # Which branch of the repository should be used when creating links (optional)

branch: mb/neurolibre # Which branch of the repository should be used when creating links (optional)

Will also have to move this release binary of the processed data to this repo or somewhere else that's not GDrive (they hate it when you access files too often):

"src": "https://github.com/shimming-toolbox/rf-shimming-7t-neurolibre/releases/download/v1/ds004906.zip",

or if you prefer, we can keep all of this in this branch and submit to NeuroLibre using a branch reference.

After this is merged/ready, submitting to NeuroLibre is done through their portal (must log in with ORCID first): https://neurolibre.org. It's a quick 2 minute form to fill out.

@mathieuboudreau
Copy link
Member Author

@jcohenadad just pinging you if the email for this PR slipped through the cracks

@jcohenadad
Copy link
Member

@jcohenadad just pinging you if the email for this PR slipped through the cracks

it's on my todo list, thank you for the reminder 😊

jcohenadad

This comment was marked as resolved.

@jcohenadad

This comment was marked as resolved.

jcohenadad

This comment was marked as resolved.

@jcohenadad
Copy link
Member

it would be cool to have the fig interactive but i guess this is too time consuming to produce (tagging @nstikov so he is aware of user-facing perception of neurolibre notebook generation):

image

Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

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

image

is the fig saving actually necessary? (it ends up in the server unused, right?)

Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

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

In reference to #91 (review), it would be great to use the fig generated by your notebook for the article. Notably because you fixed #90 on this repos, but the issue is still present on the original repos (hence my frustration of having the two repos out of sync @nstikov))-- what would be the best way to do it to get figs at 300dpi?

jcohenadad

This comment was marked as resolved.

@mathieuboudreau
Copy link
Member Author

weird formatting of the ref:

image

This is a Jupyter Book quirk because we’re using their bibtex rendering for the bibliographie, and @agahkarakuzu has told me before that we’re required to use this Jupyter Book biblio rendering for neurolibre submissions. The numbers indicate the different instances that the refs were cited in the text; to remove it I could limit to one citation instance per ref

@mathieuboudreau
Copy link
Member Author

it would be cool to have the fig interactive but i guess this is too time consuming to produce (tagging @nstikov so he is aware of user-facing perception of neurolibre notebook generation):

image

The challenge here is that I don’t think it would be possible to get an interactive Plotly figure that looks exactly this way. Plotly doesn’t have an image-visualizing tool; they refer users to their heatmap tool to display images/qMRI maps like this. The best I could achieve was to concatenate 2 subplots, but to do this with 8 may not work as desired.

@mathieuboudreau
Copy link
Member Author

In reference to #91 (review), it would be great to use the fig generated by your notebook for the article. Notably because you fixed #90 on this repos, but the issue is still present on the original repos (hence my frustration of having the two repos out of sync @nstikov))-- what would be the best way to do it to get figs at 300dpi?

There’s an optional config that can be added to do this, i’ll let u know when its added with an example outputs (its not saved inside the script, but gives a displaybar that lets you save it to your configurated format)

@mathieuboudreau
Copy link
Member Author

do we want to use the same font as the rest of the fig for 'sub-01', etc.

image

Good catch, will fix these annotations

@jcohenadad
Copy link
Member

to remove it I could limit to one citation instance per ref

no need

@jcohenadad
Copy link
Member

The challenge here is that I don’t think it would be possible to get an interactive Plotly figure that looks exactly this way. Plotly doesn’t have an image-visualizing tool; they refer users to their heatmap tool to display images/qMRI maps like this. The best I could achieve was to concatenate 2 subplots, but to do this with 8 may not work as desired.

no worries (and no need to spend time on this), i mostly wanted to give feedback for @nstikov

@mathieuboudreau
Copy link
Member Author

@jcohenadad

In reference to #91 (review), it would be great to use the fig generated by your notebook for the article. Notably because you fixed #90 on this repos, but the issue is still present on the original repos (hence my frustration of having the two repos out of sync @nstikov))-- what would be the best way to do it to get figs at 300dpi?

Updated the Plotly config to show a display bar to export a PNG:

Screenshot 2024-02-13 at 7 42 57 PM

From: https://shimming-toolbox.github.io/rf-shimming-7t/

I've exported a version of the PNG,

Figure 3_plotly

and uploaded it to the the figures folder for the manuscript (link to figure: https://drive.google.com/file/d/10uSw0ct5VATOqHcgmj8mq3FHn6qnAYs8/view?usp=sharing). If good as is, we can delete the other fig3 files.

A few notes:

  • Exporting a TIFF directly isn't possible through Plotly. If a *.tiff file is needed, you can simply rename the file (png->tiff work like that). Otherwise,
  • By changing a setting (
    " 'format': 'png', # one of png, svg, jpeg, webp\n",
    ), we can let plotly export SVG's instead, and from here you could open the vector file in Inkscape/Illustrator and export it that way.
  • In addition, the conversion process from Plotly to PNG/SVG isn't identical; there are some small differences in scaling/positionning due to whatever tool they use. If some minor resizing/moving is desired, this should be done via the SVG.
  • The proportions of the image also requires a bit of guesswork; this is set via

" 'height': 2500,\n",
" 'width': 1250,\n",
" 'scale': 2 # Multiply title/legend/axis/canvas sizes by this factor\n",

changing the height/width will change the ratio, and the final number of pixels in your images will be these numbers times your scaling factor (which we set to 2 to give higher res; I find that setting the number of pixels higher directly doesn't give as good of a final image than doing it this way).

@mathieuboudreau
Copy link
Member Author

@jcohenadad I believe I've addressed all of comments, you can have one more look here: https://shimming-toolbox.github.io/rf-shimming-7t/

Once you think this is ready for merging, I'll update all the references that point to this branch (see original post for this PR) at all locations to point to master (or is it main) instead, then it wll be ready for merging.

Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

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

beautiful work ❤️

@mathieuboudreau mathieuboudreau merged commit 09b52b0 into main Feb 14, 2024
@mathieuboudreau mathieuboudreau deleted the mb/neurolibre branch April 5, 2024 16:55
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.

Steps towards a NeuroLibre pre-print publication
2 participants