spring cleaning: removing bugs and code smells #261

1b15 · 2024-03-05T19:12:13Z

addresses Parameters provided in examples (and default parameters) does not perform BOLD simulation correctly #250 and Example files appear to be broken #254
fixes inconsistencies and unexpected behaviour in the implementation and interaction of chunking, continuing runs, bold, and appending outputs
improves maintainability by removing various code smells (e.g., low cohesion because of global bold parameter, repeated implementation of the same output saving code, etc)

codecov · 2024-03-05T19:29:31Z

Codecov Report

Attention: Patch coverage is 88.05970% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 91.07%. Comparing base (ec2434e) to head (19212bb).

Files	Patch %	Lines
neurolib/models/model.py	87.76%	6 Missing ⚠️
neurolib/models/multimodel/model.py	77.78%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #261      +/-   ##
==========================================
+ Coverage   90.93%   91.07%   +0.13%     
==========================================
  Files          65       65              
  Lines        5206     5172      -34     
==========================================
- Hits         4734     4710      -24     
+ Misses        472      462      -10

Flag	Coverage Δ
unittests	`91.07% <88.06%> (+0.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

caglorithm

Thank you very much for this awesome PR. Please have a look at the comments I left.

The only thing I can see is that continue_run might not behave as expected and that there is a change of the time axis in storeOutputsAndStates.

caglorithm · 2024-03-11T14:13:03Z

neurolib/models/bold/model.py

-        if self.BOLD.shape[1] == 0:
-            # add new data
-            self.t_BOLD = t_BOLD_resampled
-            self.BOLD = BOLD_resampled
-        elif append is True:
-            # append new data to old data
-            self.t_BOLD = np.hstack((self.t_BOLD, t_BOLD_resampled))
-            self.BOLD = np.hstack((self.BOLD, BOLD_resampled))
-        else:
-            # overwrite old data
-            self.t_BOLD = t_BOLD_resampled
-            self.BOLD = BOLD_resampled
-


You seem to have the append=True case, why?

The functionality for saving outputs is already in models/model.py.

caglorithm · 2024-03-11T14:23:12Z

neurolib/models/model.py

@@ -311,7 +299,7 @@ def clearModelState(self):
        self.state = dotdict({})
        self.outputs = dotdict({})
        # reinitialize bold model
-        if self.params.get("bold"):
+        if self.boldInitialized:


Does this do the same thing as before? Reads like: if it is initialized, initialize it.

The difference is that BOLD is now only initialized at the beginning of the first run. One only needs to clear the bold state with re-initialization if it has been initialized before.

caglorithm · 2024-03-11T14:24:05Z

neurolib/models/model.py

+        if not hasattr(self, "t"):
+            raise ValueError("You tried using continue_run=True on the first run.")


Why do we have to error here? The user could user continue_run=True in the first call as well.

addressed this in the latest commit

caglorithm · 2024-03-11T14:26:06Z

neurolib/models/model.py

-            data = data[:, :: self.sample_every]
-        else:
-            raise ValueError(f"Don't know how to subsample data of shape {data.shape}.")
+        if data.shape[-1] >= self.params["duration"] - self.startindt:


Does this if clause prevent any previous case to run?

This catches unintended subsampling of BOLD.

caglorithm · 2024-03-11T14:27:20Z

neurolib/models/multimodel/model.py

+        if continue_run:
+            self.setInitialValuesToLastState()
+        else:


We would like to allow continue_run even in the case if there hasn't been any previous run.

addressed this in the latest commit

caglorithm · 2024-03-11T14:28:05Z

neurolib/models/multimodel/model.py

+        self.setOutput("t", results.time.values, append=append, removeICs=False)
+        self.setStateVariables("t", results.time.values)


This changes the time axis.

We previously had different time axis behaviours for model and multimodel:

for regular models, the time axis always started at 0 because it was reset for every chunk unless it was appended

for multimodels, the time axis started at self.start_t from continued runs

We adopted the multimodel behaviour with a more elegant implementation without self.start_t.

VascoSch92 · 2024-03-12T10:43:05Z

neurolib/models/multimodel/model.py

-        # set start t for next run for the last value now
-        self.start_t = self.t[-1]
+        if not hasattr(self, "t"):
+            raise ValueError("You tried using continue_run=True on the first run.")


Suggested change

raise ValueError("You tried using continue_run=True on the first run.")

raise ValueError("You tried using `continue_run=True` on the first run.")

thank you Vasco, I forgot to make the suggested change in the multimodel

caglorithm

LGTM, amazing work, thank you

1b15 added 15 commits March 4, 2024 12:28

removed non-existing argument from aln docstring

9ac8b34

removed params['bold'] option

178592c

cleaned up bold initialization check

159de62

removed unreachable condition

f96cf3d

fix continue_run order

47a7cef

fix typo

8e6fa1a

clean up simulateBold

645fa01

fix continue_run for chunkwise=True

5652090

simplified outputDict.items()

a455dd9

major fixes: setOutput / BOLD

27daf4e

remove unused arguments

cd73acd

fixed aln minimal notebook

73f7b9f

fixes to multimodel, testexploration, and continue_run

448598b

reverted to automatic append for BOLD outputs

69d6aab

remove duplicate arguments

6436e20

1b15 requested review from jajcayn, caglorithm and caglarcakan March 5, 2024 19:12

1b15 self-assigned this Mar 5, 2024

removed unnecessary append after reverting to default append for BOLD

fc62d4a

caglorithm requested changes Mar 11, 2024

View reviewed changes

1b15 added 2 commits March 11, 2024 17:40

remove unused self.start_t, fix BOLD append for multimodel

4aaa8cd

allow continue_run=True on first model run

bd6dbdc

VascoSch92 reviewed Mar 12, 2024

View reviewed changes

1b15 added 2 commits March 12, 2024 12:31

allow continue_run=True on first multimodel run

85b5ea9

removed first run continue_run warnings

19212bb

1b15 requested review from caglorithm and removed request for jajcayn and caglarcakan March 12, 2024 15:08

caglorithm approved these changes Apr 2, 2024

View reviewed changes

caglorithm merged commit 6be0d37 into master Apr 2, 2024
12 checks passed

caglorithm deleted the spring_cleaning branch April 2, 2024 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spring cleaning: removing bugs and code smells #261

spring cleaning: removing bugs and code smells #261

1b15 commented Mar 5, 2024

codecov bot commented Mar 5, 2024 •

edited

Loading

caglorithm left a comment

caglorithm Mar 11, 2024

1b15 Mar 11, 2024

caglorithm Mar 11, 2024

1b15 Mar 11, 2024

caglorithm Mar 11, 2024

1b15 Mar 11, 2024

caglorithm Mar 11, 2024

1b15 Mar 11, 2024

caglorithm Mar 11, 2024

1b15 Mar 11, 2024

caglorithm Mar 11, 2024

1b15 Mar 11, 2024 •

edited

Loading

VascoSch92 Mar 12, 2024

1b15 Mar 12, 2024

caglorithm left a comment

		if not hasattr(self, "t"):
		raise ValueError("You tried using continue_run=True on the first run.")

		self.setOutput("t", results.time.values, append=append, removeICs=False)
		self.setStateVariables("t", results.time.values)

	raise ValueError("You tried using continue_run=True on the first run.")
	raise ValueError("You tried using `continue_run=True` on the first run.")

spring cleaning: removing bugs and code smells #261

spring cleaning: removing bugs and code smells #261

Conversation

1b15 commented Mar 5, 2024

codecov bot commented Mar 5, 2024 • edited Loading

Codecov Report

caglorithm 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

1b15 Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caglorithm left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 5, 2024 •

edited

Loading

1b15 Mar 11, 2024 •

edited

Loading