spring cleaning: removing bugs and code smells by 1b15 · Pull Request #261 · neurolib-dev/neurolib

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

-        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

        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

+        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

-            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

+        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

+        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

-        # 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 caglarcakan, caglorithm and jajcayn 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 caglarcakan and jajcayn March 12, 2024 15:08

caglorithm approved these changes Apr 2, 2024

View reviewed changes

caglorithm merged commit 6be0d37 into master Apr 2, 2024

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

		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.")

Uh oh!

Conversation

1b15 commented Mar 5, 2024

Uh oh!

codecov Bot commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

caglorithm 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!

1b15 Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

caglorithm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Mar 5, 2024 •

edited

Loading

1b15 Mar 11, 2024 •

edited

Loading