Data path continued - Githubissues

SyneRBI / SIRF-Exercises

SIRF Training and demonstration material

http://www.ccpsynerbi.ac.uk

Apache License 2.0

18 stars 21 forks source link

Data path continued #94

Closed ashgillman closed 3 years ago

ashgillman commented 3 years ago

Progress on #89:

[x] Fix remaining PET Notebooks
[x] chdir is absolute so can be run multiple times
[x] from sirf_exercises import exercises_data_path
[x] examples_data_path -> examples_data_path()
[x] exercise_data_path.py flagged by git - this was because .gitingore is only for new files, not modified - now we don't modify a file but create
[ ] Separate working directories from data_directories

ashgillman commented 3 years ago

Testing completed:

ran ./scripts/download_data.sh

ran ./scripts/download_data.sh data

ran ./scripts/download_data.sh data ~/tmp after copying downloaded files to ~/tmp - they are found correctly

ran all modified notebooks until the cd command and ensured it ran. Note that notebooks weren't run to completion

ashgillman commented 3 years ago

I've also tested the PET notebooks (completely here, since they were short). All work well, except MAPEM which runs, but gives some funky results - I presume unrelated.

ashgillman commented 3 years ago

To explain how this works:

each Notebook dir has a notebook_setup.py which need to be imported, and which will set the sys.path to point to SIRF-Exercises/lib
Under lib are the library directories. If we wanted, these could include plotting and other repeated functions
download_data.sh now creates lib/sirf_exercises/data_path.py with the location
exercises_data_path() looks for that file, and also checks an ENV var which could be useful in the future but currently never really would be used. If it can't find the path it raises an exception.

KrisThielemans commented 3 years ago

also a codacy suggestion for quoting and spaces

KrisThielemans commented 3 years ago

except MAPEM which runs, but gives some funky results - I presume unrelated.

ah. please create an issue...

ashgillman commented 3 years ago

Yes, I know I went overboard there... It was easier than checking line-by-line with Codeacy I think it should still be valid

Huh, I did test.. I think that slipped through based on short-circuiting. I've tested with different flags a little more rigorously. Although I also removed those particular line as it is now desirable to be able to run the script without downloading any data, if no flags are specified, if you wanted to just configure the path file.

I have no idea how to please Codeacy! If I put the function docstring to start after ``` it says Multi-line docstring summary should start at the second line (D213)" and if I put a newline it says "Multi-line docstring summary should start at the first line (D212)"!

ashgillman commented 3 years ago

Hmm, it has conflicting rules! https://stackoverflow.com/questions/45990301/pep257-d212-and-d213-conflicts

ashgillman commented 3 years ago

I personally prefer

def some_func():
   """A docstring.

    more detail.
    """
    return

which would mean disabling D213

KrisThielemans commented 3 years ago

Hmm, it has conflicting rules! https://stackoverflow.com/questions/45990301/pep257-d212-and-d213-conflicts

yes. I think the idea is that we choose one to enforce style. anyway