Introducing Two Compartment exchange Model

[MohamedNasser8]

This PR adds:

1. Model Function: Implemented the 2CXM.

2. Documentation: Added detailed documentation explaining the model.

3. Testing: Conducted some tests to validate the model’s performance and accuracy.

[plaresmedima]

Hi - I would suggest you consider divisions by zero. What will happen if vp=0 for instance and is it the expected behaviour? Note these scenarios will be very common for instance in model fitting with positivity constraints on the parameters. Raising an exception would not be the right solution I think.

…

On Sun, 1 Sept 2024, 15:38 Mohamed Nasser, ***@***.***> wrote: This PR adds: 1. Model Function: Implemented the 2CXM. 2. Documentation: Added detailed documentation explaining the model. 3. Testing: Conducted some tests to validate the model’s performance and accuracy. ------------------------------ You can view, comment on, or merge this pull request online at: #48 Commit Summary - 1639ce5 <1639ce5> Initial implementation for 2CXM - 5216dad <5216dad> Add test for 2cxm - 1a91070 <1a91070> Add some tests - 1429150 <1429150> Merge branch 'main' into Two-CXM - 6dd6763 <6dd6763> Added 2CXM documentation and example File Changes (8 files <https://github.com/OSIPI/osipi/pull/48/files>) - *M* .pre-commit-config.yaml <https://github.com/OSIPI/osipi/pull/48/files#diff-63a9c44a44acf85fea213a857769990937107cf072831e1a26808cfde9d096b9> (4) - *A* docs/examples/tissue/plot_two_cxm.py <https://github.com/OSIPI/osipi/pull/48/files#diff-39acce074d9768c1a774243b1c3509c62faa5f59706c547b2a39386eafe389f8> (38) - *A* docs/references/models/tissue_models/2cxm.md <https://github.com/OSIPI/osipi/pull/48/files#diff-be464c513c734881c29b0b0415490bcd848f6224ffe28866bf00724db6746077> (16) - *M* docs/references/models/tissue_models/index.md <https://github.com/OSIPI/osipi/pull/48/files#diff-6711979373ba39baa1300e00c40d01e67fbd6610121387c21217a3832eeb0c0e> (1) - *M* mkdocs.yml <https://github.com/OSIPI/osipi/pull/48/files#diff-98d0f806abc9af24e6a7c545d3d77e8f9ad57643e27211d7a7b896113e420ed2> (1) - *M* src/osipi/__init__.py <https://github.com/OSIPI/osipi/pull/48/files#diff-8e0fee4bfe7a6e48190241b8fc0438dcf1028de5607f20e0fc522199110d6ac7> (3) - *M* src/osipi/_tissue.py <https://github.com/OSIPI/osipi/pull/48/files#diff-366a1fc0115e839dc9535b7c16792b79b51113b8e1977f3fc9237c222f08295b> (144) - *M* tests/test_tissue.py <https://github.com/OSIPI/osipi/pull/48/files#diff-3c091d1a34267254bf668ade036aff1dcd0b1657a85736ce16063289ff1ae9d3> (27) Patch Links: - https://github.com/OSIPI/osipi/pull/48.patch - https://github.com/OSIPI/osipi/pull/48.diff — Reply to this email directly, view it on GitHub <#48>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABOFKA4MPNYSO2AN2JLN32TZUMRG5AVCNFSM6AAAAABNO4WUY2VHI2DSMVQWIX3LMV43ASLTON2WKOZSGQ4TSNJQHA4DINA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[MohamedNasser8]

Hello @plaresmedima ,
In this case, what should the output be if ​Vp=0? My understanding is that the original model equation wouldn't be valid under this condition. The compartment equation from which the model is derived would change, meaning we can't simply apply the same model equation as before.

Is that correct?

[plaresmedima]

With vp=0 you have a standard Tofts model so you can check for that and handle it separately. Has this implementation been validated in the OSIPI library? Eg. I am not convinced the use of np.convolve() is right here but I may be wrong.. Prof. Steven Sourbron, PhD Chair in Medical Imaging Physics University of Sheffield, UK *https <https://www.sheffield.ac.uk/medicine/people/iicd/steven-sourbron>://www.sheffield.ac.uk/medicine/people/iicd/steven-sourbron <https://www.sheffield.ac.uk/medicine/people/iicd/steven-sourbron>* *Sheffield abdominal imaging research page* <https://www.sheffield.ac.uk/medicine/research/research-themes/medical-imaging/medical-imaging-research/abdominal-imaging> *Open source initiative in perfusion imaging* <http://www.osipi.org> *Magnetic resonance imaging biomarkers for chronic kidney disease <https://renalmri.org/>*

…

On Tue, 3 Sept 2024 at 13:29, Mohamed Nasser ***@***.***> wrote: Hello @plaresmedima <https://github.com/plaresmedima> , In this case, what should the output be if ​Vp=0? My understanding is that the original model equation wouldn't be valid under this condition. The compartment equation from which the model is derived would change, meaning we can't simply apply the same model equation as before. Is that correct? — Reply to this email directly, view it on GitHub <#48 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABOFKA2VDC3ZCD55NYKESSLZUWTSXAVCNFSM6AAAAABNO4WUY2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRWGM4TSOJWG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[MohamedNasser8]

No, it's still waiting for review.

[plaresmedima]

Probably best to stick to code that has been reviewed and tested. Some of this detail matters a lot.

…

On Tue, 3 Sept 2024, 15:16 Mohamed Nasser, ***@***.***> wrote: No, it's still waiting for review. — Reply to this email directly, view it on GitHub <#48 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABOFKA4KLL2GJ7GYN5PQJITZUXAEPAVCNFSM6AAAAABNO4WUY2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRWGY2DINZSGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[MohamedNasser8]

Yes, I used the OSIPI DCE code collection as a reference for my implementation. Additionally, I've included a plot of the model in the documentation to help ensure it's been implemented correctly.

[ltorres6]

@plaresmedima Thanks for your input, I agree that we need to ensure proper handling of edge cases as well as non-sensical numbers (negatives, nan's, etc). I will make sure to keep my eyes peeled for these things when I get a chance to review. By chance do you have an implementation Mohamed can use as another example of the 2cx model?

@MohamedNasser8 We will want to ensure we handle the edge cases here carefully. As Steven mentions, when Vp is zero, we effectively are effectively stating that the plasma compartment in the 2 compartment model is non-existent, which simplifies the model to the Tofts variation with only the extravascular extracellular compartment.

[plaresmedima]

@luis Torres ***@***.***> we have an implementatiom in dcmri that you can use as a check https://qib-sheffield.github.io/dcmri/generated/api/dcmri.conc_tissue.html#dcmri.conc_tissue But I should note this is work in progress too. Despite my own advice we have not yet systematically checked all possible boundary cases, sich as vp=0. We tested a few - see unit tests here https://github.com/QIB-Sheffield/dcmri/blob/main/tests%2Ftest_pkmods.py Also the implementation is lazy as it uses a numerical n-compartment model solution. For 2cxm the solution is analytically known so that should be used. We have an implementation of that currently as a helper function _propagate_2cxm in here https://github.com/QIB-Sheffield/dcmri/blob/main/src%2Fdcmri%2Flib.py But the arguments and return values of that function are not yet aligned with the overall package logic. We'll get these issuss sorted in one of the next versions.

…

On Tue, 3 Sept 2024, 22:14 Luis Torres, ***@***.***> wrote: @plaresmedima <https://github.com/plaresmedima> Thanks for your input, I agree that we need to ensure proper handling of edge cases as well as non-sensical numbers (negatives, nan's, etc). I will make sure to keep my eyes peeled for these things when I get a chance to review. By chance do you have an implementation Mohamed can use as another example of the 2cx model? @MohamedNasser8 <https://github.com/MohamedNasser8> We will want to ensure we handle the edge cases here carefully. As Steven mentions, when Vp is zero, we effectively are effectively stating that the plasma compartment in the 2 compartment model is non-existent, which simplifies the model to the Tofts variation with only the extravascular extracellular compartment. — Reply to this email directly, view it on GitHub <#48 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABOFKAZFVV24DZIBEPIHH2LZUYRBTAVCNFSM6AAAAABNO4WUY2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRXGQ2DOOBXGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[LucyKershaw]

I have a few things that need looking at:

1. The osipi name is the two compartment exchange model - please change in _tissue line 377
2. Lexicon units for Fp are ml/min/100 ml tissue. You have stated ml/min/100g tissue. My code (that I think you've used) is in ml/min/ml tissue. You have stated time in units of seconds but I see no conversions between any units. Please check that the curves produced are correct.
3. Add a reference to the paper that defines the 2CXM using these parameters (TE, TB etc)
4. In test_tissue some of the comments have been cut off, for example in line 128 it says '1. Basic operation of the function - test that the peak tissue' which no longer makes sense. This has happened for test 2 and (I think) test 3
5. @plaresmedima and @MohamedNasser8 this is my implementation of the model that was compared to the others here:
   https://osipi.github.io/DCE-DSC-MRI_TestResults/2CXM.html
   It has bias so it would be better to use one of the other contributed versions instead - they all seem to match well.
6. We will need to think carefully about how to test edge cases, but I don't think we've done this extensively for the other models either. I agree that vp=0 shouldn't throw an exception. Perhaps my implantation is lazy, but for now it's the way that the lexicon defines the model so it will probably do. In future we can add more ways to calculate.

[plaresmedima]

Re 5 - yes I agree, best to stick to implementations that are in the repository and have been tested by the task force. It goes against the logic of the osipi package to fetch implementations that have not gone through this process. Prof. Steven Sourbron, PhD Chair in Medical Imaging Physics University of Sheffield, UK *https <https://www.sheffield.ac.uk/medicine/people/iicd/steven-sourbron>://www.sheffield.ac.uk/medicine/people/iicd/steven-sourbron <https://www.sheffield.ac.uk/medicine/people/iicd/steven-sourbron>* *Sheffield abdominal imaging research page* <https://www.sheffield.ac.uk/medicine/research/research-themes/medical-imaging/medical-imaging-research/abdominal-imaging> *Open source initiative in perfusion imaging* <http://www.osipi.org> *Magnetic resonance imaging biomarkers for chronic kidney disease <https://renalmri.org/>*

…

On Wed, 11 Sept 2024 at 09:21, LucyKershaw ***@***.***> wrote: I have a few things that need looking at: 1. The osipi name is the two compartment exchange model - please change in _tissue line 377 2. Lexicon units for Fp are ml/min/100 ml tissue. You have stated ml/min/100g tissue. My code (that I think you've used) is in ml/min/ml tissue. You have stated time in units of seconds but I see no conversions between any units. Please check that the curves produced are correct. 3. Add a reference to the paper that defines the 2CXM using these parameters (TE, TB etc) 4. In test_tissue some of the comments have been cut off, for example in line 128 it says '1. Basic operation of the function - test that the peak tissue' which no longer makes sense. This has happened for test 2 and (I think) test 3 5. @plaresmedima <https://github.com/plaresmedima> and @MohamedNasser8 <https://github.com/MohamedNasser8> this is my implementation of the model that was compared to the others here: https://osipi.github.io/DCE-DSC-MRI_TestResults/2CXM.html It has bias so it would be better to use one of the other contributed versions instead - they all seem to match well. 6. We will need to think carefully about how to test edge cases, but I don't think we've done this extensively for the other models either. I agree that vp=0 shouldn't throw an exception. Perhaps my implantation is lazy, but for now it's the way that the lexicon defines the model so it will probably do. In future we can add more ways to calculate. — Reply to this email directly, view it on GitHub <#48 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABOFKA2SQZ6P274G2ZMLDXDZV74PNAVCNFSM6AAAAABNO4WUY2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBSHE3TMMRUGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[MohamedNasser8]

@LucyKershaw I’ve converted all the units to the correct values and added a corner case for when vp equals zero.
Can you provide the reference for this implementation to include it in the documentation?

[LucyKershaw]

Hi Mohamed,

Are you still working on this? If so, I think this needs to change a bit more - having discussed with others I think a different implementation of this model would be better. We tested all the contributed versions and in some cases this implementation ended up with outliers (https://osipi.github.io/DCE-DSC-MRI_TestResults/2CXM.html) Obviously this is entirely my fault so if you don't want to change it feel free to assign this to me instead! The MB, MJT and OGJ versions all give similar answers over a wider set of parameter space so they would be a better addition to the package.

[MohamedNasser8]

Okay, I'll take a look into the other implementations

[MohamedNasser8]

Hello @LucyKershaw, I changed the implemented model to MJT; a lot of testing is required.
If you could please take a look; if it's good, I'll complete the tests.

[LucyKershaw]

Permeability-surface area product is PS not Ps
volume fractions are small letters not capitals
Volume fractions in CAPLEX are ml/100ml
Flow in CAPLEX is in ml/100ml/min
PS is in ml/100ml/min

[ltorres6]

@MohamedNasser8 if you have fixed these please resolve this comment

[LucyKershaw]

I think I've added comments to the files, but the units need looking at again, there's a factor of 100 that I'm unsure about and the units and symbols don't match the CAPLEX units. CAPLEX codes are missing for inputs.
Did MJT include a reference for this implementation of the model?

[MohamedNasser8]

Hey @LucyKershaw, sorry for this delay, I fixed your comments.
this is an example of the output by changing values, I want to make sure it looks right!

[LucyKershaw]

I'm sorry, I can't tell just from looking at the curves whether they're correct or not. You could try generating test curves and seeing if they match the data detailed here:
https://osipi.github.io/DCE-DSC-MRI_TestResults/2CXM.html

And in the file here:
https://github.com/OSIPI/DCE-DSC-MRI_CodeCollection/blob/develop/test/DCEmodels/data/2cxm_sd_0.001_delay_0.csv

Let me know how you go on

[MohamedNasser8]

It generates identical curves now using the same data here: https://github.com/OSIPI/DCE-DSC-MRI_CodeCollection/blob/develop/test/DCEmodels/data/2cxm_sd_0.001_delay_0.csv
But I can't find the reference to this implementation on mjt code.

[MohamedNasser8]

Hey @ltorres6, @LucyKershaw, @stadmill all, I've added some tests, but I think a lot is yet to be covered.
I validated the output using the example files Dr. Lucy provided above.

[ltorres6]

Thanks Mohamed, good start on the testing. Here are some improvements you can make for testing this function:

• You test 4 different scenarios within one function here. It's best to separate these tests into subtests for debugging purposes, as well as to avoid simply failing on the first test and not running consecutive tests.
• Two of the assertions are asserting the same thing (tissue concentration is less than AIF), just for different input arguments.
• Your test function doesn't really provide information on failures.

Can you please try the following:

• Use pytest. You'll need to ensure it's included as a dependency and imported in the tests file.
• Create separate test functions for the offset test and edge cases tests (Vp=0) from the first two (basic functionality).
• Use parameterization decorators to assign variables and expected results.

@pytest.mark.parametrize("a,b,c", [(np.linspace(0, 6 * 60, 360), 10, 5)])
def test_function_here(a,b,c):

• You also have to remember to test the invalid arguments in these cases. What happens when your variables/arguments are negative? You should create a separate test function for this.
  All in all, you should have 4 different test functions, one for the basic functionality, one for the offset test, one for the tofts behavior, and one for invalid inputs.

Please take a crack at it and reach out on slack if you have any questions.

[ltorres6]

@MohamedNasser8 if you have fixed these please resolve this comment