Matlab installation + adding Amita's code to wrapper #103

oliverchampion · 2025-05-28T06:55:34Z

Unit test will fail. Mainly checking whether matlab is called

Describe the changes you have made in this PR

Matlab installation on github
Adding Amita's code to wrapper

Link this PR to an issue [optional]

none

Checklist

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

Unit test will fail. Mainly checking whether matlab is called

- fixed initial guesses and bounds such that it passes tests - changed matlab huild for github, hopefully overcoming errors.

etpeterson

Sorry, not much time this week but took a quick look and had some thoughts.

etpeterson · 2025-06-03T05:41:39Z

.github/workflows/unit_test.yml

@@ -30,6 +30,24 @@ jobs:
        run: |
          python -m pip install --upgrade pip
          pip install -r requirements.txt
+      - name: Check out repository


There's a checkout that happens about 10 lines above. Do you want 2?

etpeterson · 2025-06-03T05:44:08Z

.github/workflows/unit_test.yml

+      - name: Set environment variables for MATLAB Engine
+	if: ${{ runner.os == 'Linux' || runner.os == 'macOS' }}
+        run: |
+          if [[ "${{ runner.os }}" == "macOS" ]]; then


I don't see this variable set in the examples. What is it for? And does windows not need it?

So, Matlab is saved at different locations depending on the OS. The matlab engine only points to the default windows location. So I need to add the matlab location to the path library for mac and linux. The first If statement is a bit obsolete, but the Windows simulator crashed on the second set of if statements, despite not needing to do anything there...

etpeterson · 2025-06-03T05:45:54Z

tests/IVIMmodels/unit_tests/test_ivim_fit.py

@@ -6,6 +6,9 @@
 import time
 from src.wrappers.OsipiBase import OsipiBase
 #run using python -m pytest from the root folder
+import matlab.engine


Shouldn't this be in the wrapper and not the test? Also the starting below.

So this is the only way I can start Matlab engine ONCE without saving the algorithms. So now I give the wrapper the option to use a pre-defined matlab engine, OR if not defined, start the engine itself. By having the engine start in testing and be re-used per algorithm, it saves 20 seconds each time the algorithm is initiated (which is about 40 times or so).

…t isntalled

when testing in all folders, all code is initiated. When matlab is not available, it gives an error.

etpeterson

Despite my comments I think it's looking really good. I think this should handle matlab well.

etpeterson · 2025-06-09T04:27:04Z

src/standardized/ASD_MemorialSloanKettering_QAMPER_IVIM.py

+    supported_initial_guess = True
+    supported_thresholds = False
+
+    def __init__(self, bvalues=None, thresholds=None, bounds=None, initial_guess=None,eng=None):


Missing a space before eng

etpeterson · 2025-06-09T04:29:04Z

src/standardized/ASD_MemorialSloanKettering_QAMPER_IVIM.py

+
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        if not self.keep_alive:


This is duplicated with the above so perhaps just a separate internal method that's called from both?

etpeterson · 2025-06-09T04:34:00Z

tests/IVIMmodels/unit_tests/test_ivim_fit.py

@@ -5,8 +5,10 @@
 import pathlib
 import time
 from src.wrappers.OsipiBase import OsipiBase
+from conftest import eng


I don't think this is encouraged. https://stackoverflow.com/questions/68738078/is-it-discouraged-to-import-conftest-py-within-test-modules

I think the approved way would be to have a session scoped fixture so it would automatically be run once for each test session. @pytest.fixture(scope="session")

OKay, so this is now "fixed". I did also move a lot of initialisation code from the tests to contest. Not sure whether that is completely where it should go, but it was in lign with other code that was there.

etpeterson · 2025-06-09T04:36:26Z

conftest.py

@@ -163,6 +178,11 @@ def algorithm_list(filename, selected, dropped):
    with algorithm_path.open() as f:
        algorithm_information = json.load(f)
    algorithms = set(algorithm_information["algorithms"])
+    for algorithm in algorithms:
+        algorithm_dict = algorithm_information.get(algorithm, {})
+        if algorithm_dict.get("requieres_matlab", {}) == True:


You've misspelled requires, though you've been consistent so it'll work as intended!

Also here though you get the value of requires_matlab, which is True or False, but the default return is an empty dict {}. Shouldn't that be False?

etpeterson · 2025-06-09T04:41:28Z

conftest.py

+    for algorithm in algorithms:
+        algorithm_dict = algorithm_information.get(algorithm, {})
+        if algorithm_dict.get("requieres_matlab", {}) == True:
+            if eng is None:


Is this really what we want to do, silently drop an algorithm if we don't have a running matlab engine? Would it be possible to mark all the tests with that algorithm as skipped instead? That way they'd be visible but not run.

I've fixed this. Now they are skipped.

etpeterson · 2025-06-09T04:44:52Z

conftest.py

+eng = None
+
+def pytest_configure(config):
+    global eng


I have another comment related to this elsewhere, but global variables aren't generally encouraged. I could easily see this clashing in some algorithm that happens to use a variable called eng.

- Moved all the initiation functions from the unit_test folder to conftest.py - Added a skip option for all matlab tests (instead of removing them) - Removed some typos - Removed "redundant" code

etpeterson

Just a few small tweaks. Looks good though!

etpeterson · 2025-06-11T04:59:08Z

conftest.py

+            tolerances = algorithm_dict.get("tolerances", {})
+            skiptime=False
+            if first == True:
+                if algorithm_dict.get("fail_first_time", {}) == True:


Here's another case of comparing an empty dict to True

etpeterson · 2025-06-11T04:59:42Z

conftest.py

+                if algorithm_dict.get("fail_first_time", {}) == True:
+                    skiptime = True
+                    first = False
+            if algorithm_dict.get("requires_matlab", False) == True:


No need to == True if it's True already then that's all you want.
requires_matlab = algorithm_dict.get("requires_matlab", False)

Yes, similar as previous one. Found this in several locations. Fixed it now

etpeterson · 2025-06-11T05:00:08Z

conftest.py

-    if len(selected) > 0 and selected[0]:
-        algorithms = algorithms & set(selected)
-    return list(algorithms)
+        if algorithm_dict.get("requires_matlab", False) == True:


No need for == True or the if here as well.

fixed at multiple location

etpeterson · 2025-06-11T05:00:29Z

conftest.py

+                "strict": algorithm_dict.get("xfail_names", {}).get(name, True)}
+            kwargs = algorithm_dict.get("options", {})
+            tolerances = algorithm_dict.get("tolerances", {})
+            if algorithm_dict.get("requires_matlab", False) == True:


Similarly == True and no if here

etpeterson · 2025-06-11T05:02:02Z

tests/IVIMmodels/unit_tests/test_ivim_fit.py

+    if requires_matlab:
+        max_time = 2
+        if eng is None:
+            print('test is here')


Print still needed?

etpeterson · 2025-06-11T05:02:22Z

tests/IVIMmodels/unit_tests/test_ivim_fit.py

+            print('test is here')
+            pytest.skip(reason="Running without matlab; if Matlab is available please run pytest --withmatlab")
+        else:
+            print('test is not here')


And print here?

etpeterson · 2025-06-11T05:03:28Z

tests/IVIMmodels/unit_tests/test_ivim_synthetic.py

    # assert save_file == "test"
+    ivim_algorithm, requires_matlab = algorithmlist
+    if requires_matlab:


Can be simplified to if requires_matlab and eng is None:

etpeterson

Nice, looks good!

oliverchampion added 7 commits May 28, 2025 08:53

Matlab installation + adding Amita's code to wrapper

f9161f2

Unit test will fail. Mainly checking whether matlab is called

Handle Matlab engine to remain open

3a5095f

Wrapper now runs for ASD Memorial Sloan Kettering. But unit tests fail

9d407fd

fixed bugs

d471105

- fixed initial guesses and bounds such that it passes tests - changed matlab huild for github, hopefully overcoming errors.

fix bug for linux and mac

648a7a6

update workflow...

8ffbeb4

Update unit_test.yml

1159bc2

etpeterson requested changes Jun 3, 2025

View reviewed changes

oliverchampion added 9 commits June 3, 2025 12:01

update unit test

c2e9bd2

fix?

aa3c5b0

Update unit_test.yml

4e2fd54

Update unit_test.yml

799920b

add flag for running matlab tests

e758974

remove matlab install from workflow

fc31bc6

fix right location for matlab avail

4e4a6dc

exclude matlab algorithms from testing algorithm list if matlab is no…

52d7deb

…t isntalled

ensure testing is only done in the test folder

b7f2f8e

when testing in all folders, all code is initiated. When matlab is not available, it gives an error.

etpeterson requested changes Jun 9, 2025

View reviewed changes

Fixed comments

c32bfbd

- Moved all the initiation functions from the unit_test folder to conftest.py - Added a skip option for all matlab tests (instead of removing them) - Removed some typos - Removed "redundant" code

etpeterson requested changes Jun 11, 2025

View reviewed changes

fixed stuff

1d48e6c

etpeterson approved these changes Jun 14, 2025

View reviewed changes

etpeterson merged commit da7e8c9 into main Jun 14, 2025
29 checks passed



		def __exit__(self, exc_type, exc_val, exc_tb):
		if not self.keep_alive:

Matlab installation + adding Amita's code to wrapper #103

Matlab installation + adding Amita's code to wrapper #103

Uh oh!

Conversation

oliverchampion commented May 28, 2025

Describe the changes you have made in this PR

Link this PR to an issue [optional]

Checklist

Uh oh!

etpeterson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpeterson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpeterson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!