search

SyneRBI / SIRF-Exercises

SIRF Training and demonstration material
http://www.ccpsynerbi.ac.uk
Apache License 2.0
17 stars 21 forks source link

Some more tidying, and using wget for the binaries #174

Closed Imraj-Singh closed 2 years ago

Imraj-Singh commented 2 years ago

Using wget from a personal onedrive file instead of storing it within the repository.

DANAJK commented 2 years ago

We also have a Zenodo community for data where you could put items for download if you wish. (Suitable only for stable items and the items remain public forever.) In the Geometry notebooks, there is a get_nifti_data notebook that uses curl to access this:

# Setup the working directory for the notebook
import notebook_setup
from sirf_exercises import cd_to_working_dir
cd_to_working_dir('Geometry')

import os

# Cet data, unzip
# curl will download into the current directory

!curl -O https://zenodo.org/record/4940072/files/nifti.zip
!unzip nifti.zip
KrisThielemans commented 2 years ago

Relying on wget and curl command line utilities is generally dangerous as you don't know if they're installed. We use curl already (also in download.sh), it'll be on the VMs/docker etc, so I'd prefer that.

at some point, we have to replace this with Python download...

Imraj-Singh commented 2 years ago

Relying on wget and curl command line utilities is generally dangerous as you don't know if they're installed. We use curl already (also in download.sh), it'll be on the VMs/docker etc, so I'd prefer that.

at some point, we have to replace this with Python download...

@KrisThielemans I have some checks to see if the files are downloaded, then use the trained model if so... Yes, I could add it to download.sh. Shall I add?

@DANAJK yes I would like to use Zenodo, but this was my "quick" fix as I haven't had time, yet... I am happy to spend some time setting up an account etc, but perhaps worth doing after PSMR?

DANAJK commented 2 years ago

Provided it is clear in the code or comments where the data is located, and where to download it to, I think it is acceptable for people to do it themselves. So, for now it’s fine to provide a OneDrive URL.

I wouldn’t mess with that download_data script at this stage.

KrisThielemans commented 2 years ago

agreed!

But @Imraj-Singh please use curl unless you're 100% sure that wget is available on the 3 systems...

Imraj-Singh commented 2 years ago

@DANAJK @KrisThielemans the wget is within one of the notebooks (the 4th), so from my understanding the download will be the same directory as the notebook. So the participant will see the model and results downloaded into the same directory as the notebook which goes onto use it.

@KrisThielemans by 3 systems do you mean windows, linux and mac, or the three servers in jupyter hub (I guess the three servers use different resources). Thanks! I can check this tomorrow later today rather quickly.

KrisThielemans commented 2 years ago

@Imraj-Singh please just replace wget with curl. Less headache! (3 systems: VM, docker, jupyterhub, and even if it's there, we probably don't guarantee it)

Imraj-Singh commented 2 years ago

@KrisThielemans that's it changed to curl, was rather involved as wget hides the use of cookies and redirect where curl you have to specify. I guess this is living and learning...

KrisThielemans commented 2 years ago

Thanks!

Ready to merge?

Imraj-Singh commented 2 years ago

yes please, the codacy changes seem rather trivial unless you'd like those fixed before merging?