Added Tofts and Ext. Tofts #5
Conversation
… and set offset time default in tissue model to 30.0 sec instead.
… and set offset time default in tissue model to 30.0 sec instead. Remove dummy plot from tofts example folder.
modified as per PR comments Modified AIF: default BAT=0, allowing tissue models to have a delay time input argument.
added lexicon code for arterial delay time
Removed dose as aif input Corrected units of inputs to tofts
Added warning for cases when Kt<=0 or ve<=0. Tofts model accepts nonuniform time spacing but added warning to indicate internal resampling.
Corrected ktrans units
Added tofts model
First try adding code, extended Tofts with examples
1) New line in _tissue.py to enable proper rendering of example 2) Added space between number and units 3) Warnings for Ktrans=0 and ve=0 removed to avoid warnings during fitting ) Extended_Tofts case for Ktrans or ve =0 changed to correct value (ca*vp) including tests
typo in test_tissue
Added extended Tofts
There was a problem hiding this comment.
Okay I've gone through the MR and have some notes/suggestions.
Overall Structure
- Nice job on setting up Sphinx and automated publishing. The docs look nice.
- I would change the name of the repository to "osipi" or "osipi_sdk" rather than pypi.
- I would change the name of plot_dummy to plot_template or something similar, indicating it's meant to provide a starting point for something rather than a dummy placeholder file.
It looks like the package structure is designed to align with the lexicon, which is great wrt nomenclature. Maybe it would be a good idea to include references to the relevant lexicon pages when submitting a pull request so that a reviewer can use that as a reference (just thinking out loud here).
Probably out of scope, but I'm not sure the package structure itself makes sense from a users POV. If I work in DCE, I don't want to sift through all of the functions in the package to find relevant models, discretization methods, etc. I think the following structure for submodules makes more sense: osipi.dce, osipi.asl, osipi.dsc could all be entrypoints for a specific set of tools in that field. osipi.utils can contain anything worth generalizing, like generic deconvolution/convolution methods or all of the signal models (GRE, SE, SPGR, etc). I think this would increase modularity, reusability, and user understanding. For example, the DRO will be able to leverage all of the signal models to generate specific contrasts. If it were me, I'd create an osipi.dro sub-module for this development, for example.
Anyway, nice job. Feel free to tag me on future PR's and shoot me a message and I'll take a look.
There was a problem hiding this comment.
You can keep this file on the repo. You can add .DS_Store as well.
|
Something wrong with Sphinx or make. I think there is some error during 'make html'. It says it can't find osipi...I am able to build the html without errors locally, so not sure why it's popping up on Github. Do you think this has to do with me deleting all the DS_Store files on the repo?! |
|
The issue is with osipi not being able to load due to scipy being a dependency, and not being installed. I was able to replicate the error locally in a clean environment. I will make an MR for the fix to this and add you as a reviewer. |
Added functionality for Tofts and ETofts modelling plus tests, documentation and examples. Merge with main will update the website to show changes.