OSIPI / osipi

Python package for perfusion MRI analysis
https://osipi.github.io/osipi/
Apache License 2.0
4 stars 8 forks source link

Add functionality to convert signal intensity to concentration #10

Closed LucyKershaw closed 5 months ago

LucyKershaw commented 8 months ago

THIS IS A TEST ISSUE CREATION, PLEASE DO NOT DO ANYTHING WITH THIS UNLESS YOU ARE THE ASSIGNED PERSON

Functions should be added to allow conversion of signal intensity to contrast agent concentration. This should be done following CAPLEX nomenclature and structure for the documentation. Note that the code folder structure doesn't exactly follow the lexicon structure (and we may reorganise this better into sub-modules in the near future) so for now please go with the following:

1. https://osipi.github.io/OSIPI_CAPLEX/perfusionModels/#LinModel_SM2 - linear model to relate signal intensity to R1 In the code structure, please create _signal.py using _tissue.py as a template and add this function using lexicon notation UNLESS you feel this would work better in the _signal_to_concentration.py file (see below) instead. I don't think it matters so long as the documentation makes sense. The documentation for this should go in Models/signal models 2. https://osipi.github.io/OSIPI_CAPLEX/perfusionModels/#SPGR%20model - SPGR signal model In the code structure, please add this to _signal.py, again using the osipi notation from the lexicon The documentation for this should go in Models/signal models 3. https://osipi.github.io/OSIPI_CAPLEX/perfusionProcesses/# - signal to concentration conversion via electromagnetic property, (which in this case will of course be delta R1) In the code structure, please create _signal_to_concentration.py and add this function using lexicon notation The documentation for this should go in perfusion processes/concentration/

Don't worry too much about the precise structure of the documentation, so long as the example and descriptions are there we can finesse this afterwards as the process becomes clearer. Also if you have strong feelings that I've made a mistake in my use of the lexicon please feel free to discuss here. I think it can become quite complex because there are names for general processes and more specific ones, so I certainly don't claim to be an expert.

stadmill commented 8 months ago

@mjt320 Hi Michael, just checking to see if you wanted to discuss anything on this.

mjt320 commented 8 months ago

Hi @stadmill . I've added one of the methods in my fork. Still to add doc + test then I'll ask you both for feedback but feel free to comment anytime. One thing I've been pondering: I'm sticking to your "keep it simple" approach such that we convert 1 signal value to 1 concentration value. A downside is that we miss out on the advantage of numpy vectorisation and potentially end up with code that is slow. One possibility would be to define the minimum unit of data as 1 time series rather than 1 data point. On the other hand, if the code is mainly intended as a kind of benchmark/standard then it doesn't matter so much. Perhaps you and @LucyKershaw have discussed this already?

LucyKershaw commented 8 months ago

I would strongly favour having a vector of time series data, in real life that's the basic data unit as you say. I think otherwise the link between the models (that take in a time series) and the conversion process would get quite convoluted and, I agree - verrrrryyyyyyyy slooooooooowwwwwwww

mjt320 commented 8 months ago

I've submitted a pull request addressing part 1. I'll await feedback before addressing 2 and 3

LucyKershaw commented 7 months ago

This look fine to me. @stadmill, could you sanity check before I approve?

stadmill commented 7 months ago

Sorry I must be missing something. Just wondering why the conversion is from R1 to S...in a typical pipeline, we would acquire signal and convert it to R1.

LucyKershaw commented 7 months ago

This is a Lexicon issue I think. If we follow the way that things are set out there then this is the function that describes the relationship between signal and R1.

I think I was trying to make sure that all the pieces were there for step (3) which is the actual conversion. If we have the forward model as described in the Lexicon perhaps it would be good to also have the reverse model in the same file so that it can be reused for other bits of the pipeline, including part (3). I don't think that the Lexicon has a code for the reverse model.

stadmill commented 7 months ago

Ok yes I see now. I think the code is fine to approve Lucy.

mjt320 commented 6 months ago

Hi @stadmill and @LucyKershaw I've submitted a PR covering the other functionality you mentioned. It was an interesting exercise and raises some issues around package structure e.g. "signal to concentration conversion via electromagnetic property" is a two-step process (S->R1->C). For now we have only 1 way of doing each step and I've written: 1 x S-R1 method 1 x R1-C method 1 x method combining the two above

In future, we'll either need a separate method for each combination of steps or (e.g.) the choice of steps could be supplied as parameters.

I haven't addressed what happens when some of the input/intermediate/output values are negative, infinite, undefined, missing etc. Thought it might need some discussion to ensure consistency across functionality.