Generate a variety of testing data and came up command_line version of sim_vim_sig.py #51

Unique-Usman · 2024-03-04T17:04:11Z

A clear and concise description of what you want to happen
Added this two files.
sim_ivim_sig_command_line.py - This is the command line version of the sim_vim_sig,py
sim_ivim_sig_mul_bval.py - This version is used for running multiple b_value which are stored in b_values.py

Various test files based on the multiple value of b_value in the b_values.py
├── generic_five.json
├── generic_four.json
├── generic_original.json
├── generic_three.json
├── generic_two.json

Also, the generic_original.json was supposed to be the same thing as generic.json. But, the result is not the same. I guess the generic.json was generate from another b_value different from the current one.

Also, @etpeterson what do you think about combining the three scripts(sim_ivim_sig_command_line.py, sim_ivim_sig_mul_bval.py, sim_ivim_sig.py) together instead of having multiple scirpts doing almost similar thing ? What we can do is to just have a single script that can be used as command line tools and is able to generate multiple value of test data based on the multiple b_values.

I will update the pr based on your feeedback and also come up with a documentation for running the scipt.

Link this PR to an issue [optional]

Fixes #41

Checklist

Self-review of changed code
Added automated tests where applicable
Update Docs & Guides

…f sim_vim_sig.py

etpeterson · 2024-03-05T05:45:17Z

Yes, I think it should just be one script, just changes to the original sim_ivim_sig.py would be great. One of the inputs could be a choice for bvalues to generate.

The change to generic.json looks like somehow the newline at the end got dropped, but the contents are the same. It's hard to say how this happened because it's difficult to see changes with the duplicated script. Once everything is all together then it'll be easier to look at the changes made.

…f sim_vim_sig.py

Unique-Usman · 2024-03-05T17:01:11Z

A clear and concise description of what you want to happen
I have converted the script to one sim_ivim_sig.py.
Now it is able to take multiple bvalues through a json file. Also it can also take a single value through command line. The default value is also present.

Various test files based on the multiple value of b_value in the b_values.py
├── generic_0.json
├── generic_1.json
├── generic_2.json
├── generic_3.json
├── generic_4.json

Also, the generic_0.json was supposed to be the same thing as generic.json as I used the placeholder bvalue for it. But, the result is not the same. I guess the generic.json was generate from another b_value different from the current one. (I did not make any change to generic.json.)

There might also be a need for me to explain the changes made through a call or recording @etpeterson

I will update the pr based on your feeedback and also come up with a documentation for running the scipt.

Link this PR to an issue [optional]
Fixes #41

Checklist
Self-review of changed code
Added automated tests where applicable
Update Docs & Guides

etpeterson

Getting there. Several specific comments and it looks like one of the json files isn't correctly generated.

etpeterson · 2024-03-06T05:31:27Z

phantoms/MR_XCAT_qMRI/sim_ivim_sig.py

@@ -103,7 +105,7 @@ def contrast_curve_calc():
    D[36] = 3e-3  # 36 artery
    D[37] = 3e-3  # 37 vein
    D[40] = 1.31e-3  # 40 asc lower intestine : Hai-Jing et al. doi: 10.1097/RCT.0000000000000926
-    D[41] = 1.31e-3  # 41 trans lower intestine : Hai-Jing et al. doi: 10.1097/RCT.0000000000000926
+    D[41] = 1.31e-3  # 41 trans lower# If conversion fails, try splitting the input as a lis intestine : Hai-Jing et al. doi: 10.1097/RCT.0000000000000926


Probably accidental add?

Yeah, it is accidental. Will correct it.

etpeterson · 2024-03-06T05:34:13Z

phantoms/MR_XCAT_qMRI/sim_ivim_sig.py

+    parser.add_argument("-b", "--bvalue", type=float,
+                        nargs="+",
+                        help="B values (list of of numbers)")
+    parser.add_argument("-bf", "--bvalues-file", metavar="FILE", type=parse_bvalues_file,


Maybe just -f for the short?

Yeah, noted. Will make changes to it.

etpeterson · 2024-03-06T05:36:20Z

phantoms/MR_XCAT_qMRI/b_values.json

@@ -0,0 +1,7 @@
+[
+    [0.0, 1.0, 2.0, 5.0, 10.0, 20.0, 30.0, 50.0, 75.0, 100.0, 150.0, 250.0, 350.0, 400.0, 550.0, 700.0, 850.0, 1000.0],


Rather than a list of lists perhaps an object so they can have names rather than just numbers?

I tried object for the naming, but, we will have to enforce a pattern of naming on any one running the command for easy parsing.

Something like { generic_ : [], ...}. That is why I made it into a list of list. But, if you think object is better approach then I can change it to that.

I'd prefer objects. That way we have meaningful file names, like generic_short.json.

Some naming system would be good. I'm not sure what you mean "for easy parsing". I would imagine that people give either no name and they get all out or they give a name and they get that one out.

This is still pending as well.

Ok, I will change it to object.

etpeterson · 2024-03-06T05:36:57Z

tests/IVIMmodels/unit_tests/generic.json

I don't understand your comment that the file changed. It looks like just a newline to me, am I wrong?

Along those lines, whichever file is a duplicate of this should be deduplicated. Your changes shouldn't alter the original bvalues in any way.

The generic.json does not change. What I am saying is that, when I was trying to regenerate the generic.json using the current bvalues. I got different json values. So, that is why I said the generic.json might be generated from another bvalues different from the placeholders.

There shouldn't be changes to generic.json. I commented elsewhere on that. If it also changed in the main branch then perhaps, but I would be very surprised if that were the case.

Assuming there aren't changes though, we don't need two copies of generic.json. So confirming a duplicate and remove the original generic.json and point everything to the new one.

This is still pending.

Ohh, I will take care of that.

etpeterson · 2024-03-06T05:40:46Z

phantoms/MR_XCAT_qMRI/sim_ivim_sig.py

    else:
-        output_file = 'output_resp.nii.gz'  # Replace with your desired output file name
+        bvalues = [[0., 1, 2, 5, 10, 20, 30, 50, 75, 100, 150, 250, 350, 400, 550, 700, 850, 1000]]


It seems odd to have a double array, why is this?

I had to make it a double array so that I can have one implementation with when the value is gotten from the files. Check line 420.

Oh I see. I guess it'll change with an object implementation anyway.

etpeterson · 2024-03-06T05:44:33Z

tests/IVIMmodels/unit_tests/generic_0.json

+    },
+    "config": {
+        "bvalues": [
+            43.0,


Something isn't right with these bvalues.

Ohh, that is true, I will change it. Those values I gave while testing the script in the command line.

Unique-Usman · 2024-03-06T08:05:00Z

@etpeterson Kindly go through the comment, then I can make the necessary changes. Thank you.

Unique-Usman · 2024-03-06T13:17:50Z

I changed the generic_0.json. Also correct the mistake in the script. Also changed the -bf option flag to -f flag.

What I was saying regarding the generic.json is that, the value in generic_0.json was supposed to be the same thing with the generic.json. Because, the bvalue for generic_0.json was the one bvalue in the repo. My suggestion is that, the bvalue used to generate generic.json is different from the one present in the repo.

etpeterson · 2024-03-07T05:03:09Z

What I was saying regarding the generic.json is that, the value in generic_0.json was supposed to be the same thing with the generic.json. Because, the bvalue for generic_0.json was the one bvalue in the repo. My suggestion is that, the bvalue used to generate generic.json is different from the one present in the repo.

Is this still different? We can see the bvalues used for generic.json in the file itself. And I can see they are the same as the default set you're using. Is it different on the main branch too? I would be surprised if it's different on the main branch so that would be a good place to check.

Unique-Usman · 2024-03-07T05:42:20Z

@etpeterson , I ran it on the unchaged main branch and it still give the different result from the generic.json. Yeah, the bvalues used is the same, something is wrong somewhere I guess.

etpeterson · 2024-03-07T05:50:54Z

@etpeterson , I ran it on the unchaged main branch and it still give the different result from the generic.json. Yeah, the bvalues used is the same, something is wrong somewhere I guess.

That is surprising. What's the difference?

Unique-Usman · 2024-03-07T06:03:08Z

All the decimal values

etpeterson · 2024-03-07T16:49:41Z

This is strange. I don't know why that would change. The history doesn't show meaningful changes to that file. I'll give it a shot.

Also, unfortunately you have a merge conflict.

etpeterson · 2024-03-07T17:07:58Z

I'm not sure what to say, but it worked for me, I got the same data in both. Here are my steps.

from the phantoms/MR_XCAT_qMRI folder
git checkout main
git pull
python sim_ivim_sig.py
let it run and in a minute or two it finished
git status shows no changes
diff generic.json ../../tests/IVIMmodels/unit_tests/generic.json shows no changes

Unique-Usman · 2024-03-07T17:21:28Z

Did you move the utitlities to the phantoms/MR_XCAT_qMRI when running the code ?

etpeterson · 2024-03-07T17:35:01Z

Did you move the utitlities to the phantoms/MR_XCAT_qMRI when running the code ?

No, if the pythonpath is set correctly it finds them where they are. Even if you pull the images manually I think the result should be the same though.

Unique-Usman · 2024-03-07T17:41:34Z

Yeah, I was able to get the same data. But, do we want to add https://github.com/OSIPI/TF2.4_IVIM-MRI_CodeCollection/blob/main/phantoms/MR_XCAT_qMRI/sim_ivim_sig.py#L13 to the script. This keeps downloading all the data everytime.

etpeterson · 2024-03-08T16:42:57Z

This keeps downloading all the data everytime.

If that's the case then it sounds like a good issue to be fixed in a different pull request.

etpeterson · 2024-03-08T16:44:44Z

I'll try to actually run it later and test that, but the code looks good to me at the moment. Just a few changes still pending.

etpeterson · 2024-03-08T16:45:09Z

Yeah, I was able to get the same data

What was the problem? Is it something that we can fix with some instructions or code?

Unique-Usman · 2024-03-08T16:49:20Z

This keeps downloading all the data everytime.

If that's the case then it sounds like a good issue to be fixed in a different pull request.

I think, if possible, we should have the downloading of data separately. Then, in the documentation for running the code that I will be providing, I will mention it there.

Unique-Usman · 2024-03-08T16:51:31Z

f that's the case then it sounds like a good issue to be fixed in a different pull request.

I really think it is some mix up somewhere, probably I deleted or add some code mistakenly. I resolved the problem by rewriting the code from the start.

etpeterson · 2024-03-09T00:05:24Z

I think, if possible, we should have the downloading of data separately. Then, in the documentation for running the code that I will be providing, I will mention it there.

We shouldn't try to reimplement git-lfs here, but also a simple check to see if the requisite files are present and if not then download seems pretty easy. If it's in the document and in the code then requiring two steps is fine.

Unique-Usman added 3 commits March 4, 2024 22:02

Generate a variety of testing data and came up command_line version o…

536d9ca

…f sim_vim_sig.py

Generate a variety of testing data and came up command_line version o…

b387aa3

…f sim_vim_sig.py

Added the b_values.py

ee0b3b7

Unique-Usman added 2 commits March 5, 2024 22:23

Generate a variety of testing data and came up command_line version o…

f60f04f

…f sim_vim_sig.py

Generate a variety of testing data and came up command_line version o…

3868cc1

…f sim_vim_sig.py

etpeterson requested changes Mar 6, 2024

View reviewed changes

Updated the generic_0.json and corrected the sim_ivim_sig.py

667cbf5

Unique-Usman requested a review from etpeterson March 6, 2024 13:17

Merge branch 'main' into usman

76b5533

Unique-Usman mentioned this pull request Mar 7, 2024

Generate a variety of testing data and came up command_line version of sim_vim_sig.py #55

Merged

Unique-Usman closed this Mar 9, 2024

Unique-Usman deleted the usman branch March 12, 2024 10:26

		@@ -0,0 +1,7 @@
		[
		[0.0, 1.0, 2.0, 5.0, 10.0, 20.0, 30.0, 50.0, 75.0, 100.0, 150.0, 250.0, 350.0, 400.0, 550.0, 700.0, 850.0, 1000.0],

Generate a variety of testing data and came up command_line version of sim_vim_sig.py #51

Generate a variety of testing data and came up command_line version of sim_vim_sig.py #51

Conversation

Unique-Usman commented Mar 4, 2024

Link this PR to an issue [optional]

Checklist

etpeterson commented Mar 5, 2024

Unique-Usman commented Mar 5, 2024 • edited Loading

etpeterson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Unique-Usman commented Mar 6, 2024

Unique-Usman commented Mar 6, 2024

etpeterson commented Mar 7, 2024

Unique-Usman commented Mar 7, 2024 • edited Loading

etpeterson commented Mar 7, 2024

Unique-Usman commented Mar 7, 2024

etpeterson commented Mar 7, 2024

etpeterson commented Mar 7, 2024

Unique-Usman commented Mar 7, 2024

etpeterson commented Mar 7, 2024

Unique-Usman commented Mar 7, 2024

etpeterson commented Mar 8, 2024 • edited Loading

etpeterson commented Mar 8, 2024 • edited Loading

etpeterson commented Mar 8, 2024

Unique-Usman commented Mar 8, 2024

Unique-Usman commented Mar 8, 2024

etpeterson commented Mar 9, 2024

Unique-Usman commented Mar 5, 2024 •

edited

Loading

Unique-Usman commented Mar 7, 2024 •

edited

Loading

etpeterson commented Mar 8, 2024 •

edited

Loading

etpeterson commented Mar 8, 2024 •

edited

Loading