ENH: Make models inherit from base model #176

jhlegarreta · 2024-04-16T23:50:14Z

Make models inherit from base model.

jhlegarreta · 2024-04-27T14:47:28Z

@effigies @oesteban Following this comment #166 (comment), I had a look at inheriting the models from ModelBase. Things were unclear, and I ended up adding my questions as comments in the code. Not good practice for reviewing, but would be grateful if you had a look and commented.

The main point is that the BaseClass' fit and predict methods contain a lot of code that is not being call at all. The initialization method is not used either. The latter may be easy to fix with a call to the superclass init from the child classes, but it is unclear how the code in the fit and predict methods of BaseClass should be reused.

This is also related to issue nipreps/nifreeze#16.

So comments would be appreciated.

oesteban

I think this will make the code much more readable -- left some comments.

src/eddymotion/model/base.py

jhlegarreta · 2024-04-30T23:19:35Z

src/eddymotion/model/base.py


 def _exec_fit(model, data, chunk=None):
    retval = model.fit(data)
    return retval, chunk


-def _exec_predict(model, gradient, chunk=None, **kwargs):
+def _exec_predict_dwi(model, gradient, chunk=None, **kwargs):


I renamed this method to contain the dwi label, as it requires a gradient and optionally uses a S0 argument. If gradient in reality should be an index, it may be renamed back. Also, as things are right now, I do not see the need to pass an S0 either.

I think we should move this back to be general, and make gradient an index -- will work on this through a PR to this branch.

src/eddymotion/model/base.py

jhlegarreta · 2024-04-30T23:39:08Z

src/eddymotion/model/base.py


-        gradient = _rasb2dipy(gradient)
+        self._gtab = _rasb2dipy(self._gtab)


Not sure about this: gradient contains the whole gtab according to _rasb2dipy: https://github.com/nipreps/eddymotion/pull/176/files#diff-a875f501910044a7d95658fb83740e2c5c6c1693e7e6808703d282441db82be8L412

See also https://github.com/nipreps/eddymotion/pull/176/files#diff-a875f501910044a7d95658fb83740e2c5c6c1693e7e6808703d282441db82be8R203

I think there is a naming confusion here: the models expect a RAS+b gradient object (which is different from the dipy gtab object) into a gtab parameter. Am I correct @oesteban ?

jhlegarreta · 2024-04-30T23:41:31Z

src/eddymotion/model/base.py

        """Predict asynchronously chunk-by-chunk the diffusion signal."""
        if self._b_max is not None:
-            gradient[-1] = min(gradient[-1], self._b_max)
+            index[-1] = min(index[-1], self._b_max)


I think this is not OK. If the gradients are capped, not sure how the indices get affected/how they should be checked.

This is capping the b value (only the last item of gradient. For some models, very high b-values 'saturate' and it's better to model as if they were lower. This only kicks in after setting b_max so you need to be explicit about it.

jhlegarreta · 2024-04-30T23:44:06Z

src/eddymotion/model/base.py

-            ((gtab[3] >= self._th_low) & (gtab[3] <= self._th_high))
-            if gtab is not None
+            ((self._gtab[3] >= self._th_low) & (self._gtab[3] <= self._th_high))
+            if self._gtab is not None
            else np.ones((data.shape[-1],), dtype=bool)


I'd dare to say that self._gtab will not be None, so this if/else block is not necessary to me.

Here we do want to use the input gtab, as opposed to the global gtab, which potentially contains the left-out gradient.

src/eddymotion/model/base.py

jhlegarreta · 2024-04-30T23:53:43Z

Gave this another go. Adjusted some docstrings.

More questions/comments (inline and below, long to digest, sorry):

I am not sure whether the parent slots are inherited by the child classes
I am not sure, and how the AverageDWModel __init__ method kwargs documentation is understood, as (i) it only contains kwargs (we may need to tell something like kwargs can contain... - not sure how this is done properly in Sphinx); (ii) the parameters are defaulted to some values if not found. So maybe they can just be list as regular keyword arguments.
Not sure if I follow the timepoint/index rationale for the PET model. Do we have multiple PET volumes for a single session? If we do not, then I am not sure to follow: this would be like requiring DWI volumes from different sessions/timepoints to be aligned, which is different from what we try to do for the DWI case (correcting data from the same session).

Edit: understood after talking to Martin. So essentially, the case is the same for PET: multiple 3D volumes in one session, each taken at some interval. So much like different gradient directions for DWI.
The following:
https://github.com/nipreps/eddymotion/pull/176/files#diff-a875f501910044a7d95658fb83740e2c5c6c1693e7e6808703d282441db82be8R117

Looks like it does not apply to PET (see https://github.com/nipreps/eddymotion/pull/176/files#diff-a875f501910044a7d95658fb83740e2c5c6c1693e7e6808703d282441db82be8R382). So should it be moved to the DWI base class?

Edit: could apply to PET, so no need to pay attention for now.

jhlegarreta · 2024-05-15T13:23:57Z

Sorry to ping you again this morning @oesteban.

jhlegarreta · 2024-05-27T20:48:38Z

src/eddymotion/model/base.py

-
-        model_str = getattr(self, "_model_class", None)
-        if not model_str:
-            raise TypeError("No model defined")


@oesteban Some tests are now failing because the _model_class is None in this base class, and I am not setting any particular value in the derived classes. What is this property supposed to contain?

e.g.
https://app.circleci.com/pipelines/github/nipreps/eddymotion/1070/workflows/c9747d35-0cb1-49d9-963f-207d20887ce8/jobs/1038

I am confused about the use of this. I now see that _model_class and _modelargs are properties of the wrappers DTIModel and DKIModel. This adds to this docstring https://github.com/nipreps/eddymotion/pull/176/files#diff-a875f501910044a7d95658fb83740e2c5c6c1693e7e6808703d282441db82be8L79

If I say here

kwargs = {k: v for k, v in kwargs.items() if k in self._modelargs} from importlib import import_module model_str = "eddymotion.model.AverageDWModel" module_name, class_name = model_str.rsplit(".", 1) my_model = getattr(import_module(module_name), class_name)(**kwargs)

and leaving aside that AverageDWModel requires a gtab (as it inherits from BaseDWIModel; setting it to None in its init would make it), the above statement produces a recursive call, since instantiating AverageDWModel calls the superclass init method.

So I am not following what was intended with this block.

Also, I am not sure what we want to do here with the DTI and DKI wrappers either.

Edit: if the DTI/DKI wrappers make sense here, it looks as if the BaseDWIModel should not inherit from BaseModel, or at least, the init method of the latter and its docstring suggest that it is intended to be a superclass for the wrapper classes; however, the TrivialB0Model, AverageDWModel, etc. are not intended to be wrappers around dipy objects, and it does not make sense IMO for them to have _model_class and _modelargs properties. So there seems to be 2 things that are mixed here. The model factory will also need to be adapted following all this.

@oesteban Can you please clarify these aspects?

and leaving aside that AverageDWModel requires a gtab (as it inherits from BaseDWIModel;

The other two tests fail because of this reason.

I'll have a look ASAP - sorry for my slow turnaround

Sorry for the delayed response. I can now answer this question. I'm sorry for this particular case—you are prey to an undocumented feature.

The idea was that models can be fit in two ways:

Pure leave-one-out fashion: at every iteration of the Estimator, a fully-fledged model is fit without the particular index/orientation. This is typically very slow.

Single model: the model is fit on all the data, and each iteration produces the left-out index. These are enable by adding the prefix Full to the model name.

This is implemented in the Estimator, under the understanding that the model is the same, what changes is how you use it.

eddymotion/src/eddymotion/estimator.py

Lines 117 to 135 in ce8de17

single_model = model.lower() in (

"b0",

"s0",

"avg",

"average",

"mean",

) or model.lower().startswith("full")

dwmodel = None

if single_model:

if model.lower().startswith("full"):

model = model[4:]

# Factory creates the appropriate model and pipes arguments

dwmodel = ModelFactory.init(

model=model,

**kwargs,

)

dwmodel.fit(dwdata.dataobj, n_jobs=n_jobs)

ATM I cannot comment on why this has some effect on the model itself so a _model_class is necessary, I can't recall the reason. I bet it is just to inform the estimator that fit should not be called every time (which probably should be handled here!)

That said, let's take the average model for example. When instantiated as FullAverage, then it is fit only once before entering the iterator loop of the estimator. If not, at every iteration an average without the particular direction will be calculated in the fit call.

Okay, I'll leave my above comment because it explains something useful --- but it is totally unrelated to @jhlegarreta's question. Apologies for the confusion.

After working on the PR and re-reading the code, I understand that _model_class and _modelargs enable using DIPY models without much overhead (see DKI and DTI at the end).

oesteban

I have to checkout this code to make a better review of it. A nit pick for the time being.

src/eddymotion/model/base.py

oesteban · 2024-06-03T12:36:58Z

src/eddymotion/model/base.py

-
-        model_str = getattr(self, "_model_class", None)
-        if not model_str:
-            raise TypeError("No model defined")


I'll have a look ASAP - sorry for my slow turnaround

Make models inherit from base model.

oesteban · 2024-06-07T14:47:24Z

Let's get #166 over the final line and then I move onto this.

oesteban

I like where this is going. I'm going to add the docstring of constants and then work locally on this PR.

src/eddymotion/model/base.py

@jhlegarreta

Improving the documentation of constants. cc/ @jhlegarreta

oesteban

I responded to the two major questions in this PR. Happy to chat about the _model_class as it seems the feature may not be implemented in an intuitive way and it is definitely not sufficiently documented.

oesteban · 2024-06-08T06:52:15Z

src/eddymotion/model/base.py


 def _exec_fit(model, data, chunk=None):
    retval = model.fit(data)
    return retval, chunk


-def _exec_predict(model, gradient, chunk=None, **kwargs):
+def _exec_predict_dwi(model, gradient, chunk=None, **kwargs):


I think we should move this back to be general, and make gradient an index -- will work on this through a PR to this branch.

oesteban · 2024-06-08T07:03:34Z

src/eddymotion/model/base.py

-
-        model_str = getattr(self, "_model_class", None)
-        if not model_str:
-            raise TypeError("No model defined")


Sorry for the delayed response. I can now answer this question. I'm sorry for this particular case—you are prey to an undocumented feature.

The idea was that models can be fit in two ways:

Pure leave-one-out fashion: at every iteration of the Estimator, a fully-fledged model is fit without the particular index/orientation. This is typically very slow.

Single model: the model is fit on all the data, and each iteration produces the left-out index. These are enable by adding the prefix Full to the model name.

This is implemented in the Estimator, under the understanding that the model is the same, what changes is how you use it.

eddymotion/src/eddymotion/estimator.py

Lines 117 to 135 in ce8de17

single_model = model.lower() in (

"b0",

"s0",

"avg",

"average",

"mean",

) or model.lower().startswith("full")

dwmodel = None

if single_model:

if model.lower().startswith("full"):

model = model[4:]

# Factory creates the appropriate model and pipes arguments

dwmodel = ModelFactory.init(

model=model,

**kwargs,

)

dwmodel.fit(dwdata.dataobj, n_jobs=n_jobs)

ATM I cannot comment on why this has some effect on the model itself so a _model_class is necessary, I can't recall the reason. I bet it is just to inform the estimator that fit should not be called every time (which probably should be handled here!)

That said, let's take the average model for example. When instantiated as FullAverage, then it is fit only once before entering the iterator loop of the estimator. If not, at every iteration an average without the particular direction will be calculated in the fit call.

src/eddymotion/model/base.py

jhlegarreta · 2024-06-08T17:43:47Z

@oesteban Have gone through the comments. Will wait after this #176 (comment).

The main difficulty to make this work now lies in https://github.com/nipreps/eddymotion/pull/176/files#r1616403642. Although you answered to the thread, not sure if the question was addressed: the point is that I do not see why DTIModel and DKIModel exist in here; if they are meant to be removed, the issue related to _model_class would go away, and the inheritance would be easier I think, as it is the e.g. TrivialB0Model and AverageDWModel DWI models the ones that we are interested in subclassing from BaseDWIModel<-BaseModel.

* enh: revise code * sty: ruff format

jhlegarreta · 2024-06-12T21:13:43Z

@oesteban Had to adjust the test:
https://github.com/nipreps/eddymotion/pull/176/files#diff-b7713b8731a4d1f895063e58f31f8ab54f7bb6ca0e2b1712d85683874d94a6e6R44-R57

due to this now being applied to the TrivialB0Model:
https://github.com/nipreps/eddymotion/pull/176/files#diff-a875f501910044a7d95658fb83740e2c5c6c1693e7e6808703d282441db82be8R284-R287

src/eddymotion/model/base.py

Do not overwrite the gradient table in prediction. Co-authored-by: Oscar Esteban <[email protected]>

oesteban reviewed Apr 30, 2024

View reviewed changes

src/eddymotion/model/base.py Outdated Show resolved Hide resolved

src/eddymotion/model/base.py Outdated Show resolved Hide resolved

src/eddymotion/model/base.py Outdated Show resolved Hide resolved

src/eddymotion/model/base.py Outdated Show resolved Hide resolved

jhlegarreta force-pushed the InheritModelsFromBase branch 2 times, most recently from df51602 to 5d58f56 Compare April 30, 2024 23:16

jhlegarreta commented Apr 30, 2024

View reviewed changes

src/eddymotion/model/base.py Outdated Show resolved Hide resolved

jhlegarreta commented Apr 30, 2024

View reviewed changes

src/eddymotion/model/base.py Outdated Show resolved Hide resolved

jhlegarreta commented Apr 30, 2024

View reviewed changes

src/eddymotion/model/base.py Show resolved Hide resolved

jhlegarreta commented Apr 30, 2024

View reviewed changes

src/eddymotion/model/base.py Show resolved Hide resolved

jhlegarreta commented May 27, 2024

View reviewed changes

jhlegarreta mentioned this pull request May 30, 2024

ENH: Add gradient encoding direction angle computation utils #200

Merged

oesteban reviewed Jun 3, 2024

View reviewed changes

ENH: Make models inherit from base model

324d2ee

Make models inherit from base model.

jhlegarreta force-pushed the InheritModelsFromBase branch from 5d58f56 to 324d2ee Compare June 3, 2024 23:48

Merge branch 'main' into InheritModelsFromBase

a2d0f98

oesteban reviewed Jun 8, 2024

View reviewed changes

Apply suggestions from code review

eb4d274

Improving the documentation of constants. cc/ @jhlegarreta

oesteban reviewed Jun 8, 2024

View reviewed changes

Update src/eddymotion/model/base.py

a30b1dd

oesteban mentioned this pull request Jun 9, 2024

ENH: Implement Gaussian Process #188

Merged

jhlegarreta pushed a commit to jhlegarreta/eddymotion that referenced this pull request Jun 12, 2024

Code review of nipreps#176 (#6)

47cd1e6

* enh: revise code * sty: ruff format

jhlegarreta pushed a commit to jhlegarreta/eddymotion that referenced this pull request Jun 12, 2024

Code review of nipreps#176 (#6)

250d23a

* enh: revise code * sty: ruff format

jhlegarreta force-pushed the InheritModelsFromBase branch from 47cd1e6 to 250d23a Compare June 12, 2024 18:00

jhlegarreta pushed a commit to jhlegarreta/eddymotion that referenced this pull request Jun 12, 2024

Code review of nipreps#176 (#6)

abae1cc

* enh: revise code * sty: ruff format

jhlegarreta force-pushed the InheritModelsFromBase branch from 250d23a to abae1cc Compare June 12, 2024 20:01

jhlegarreta pushed a commit to jhlegarreta/eddymotion that referenced this pull request Jun 12, 2024

Code review of nipreps#176 (#6)

ee73cf9

* enh: revise code * sty: ruff format

jhlegarreta force-pushed the InheritModelsFromBase branch from abae1cc to ee73cf9 Compare June 12, 2024 20:30

jhlegarreta pushed a commit to jhlegarreta/eddymotion that referenced this pull request Jun 12, 2024

Code review of nipreps#176 (#6)

104bc3d

* enh: revise code * sty: ruff format

jhlegarreta force-pushed the InheritModelsFromBase branch from ee73cf9 to 104bc3d Compare June 12, 2024 21:09

Code review of nipreps#176 (#6)

a768e72

* enh: revise code * sty: ruff format

jhlegarreta force-pushed the InheritModelsFromBase branch from 104bc3d to a768e72 Compare June 12, 2024 21:12

jhlegarreta marked this pull request as ready for review June 12, 2024 21:13

oesteban reviewed Jun 12, 2024

View reviewed changes

src/eddymotion/model/base.py Outdated Show resolved Hide resolved

oesteban reviewed Jun 13, 2024

View reviewed changes

src/eddymotion/model/base.py Outdated Show resolved Hide resolved

oesteban reviewed Jun 13, 2024

View reviewed changes

src/eddymotion/model/base.py Outdated Show resolved Hide resolved

BUG: Do not overwrite the gradient table in prediction

d537833

Do not overwrite the gradient table in prediction. Co-authored-by: Oscar Esteban <[email protected]>

oesteban approved these changes Jun 13, 2024

View reviewed changes

oesteban merged commit 8c0bf36 into nipreps:main Jun 13, 2024
6 checks passed

jhlegarreta deleted the InheritModelsFromBase branch June 13, 2024 13:13

jhlegarreta mentioned this pull request May 16, 2024

Motion estimator class (estimator.py) could be made more generic to accommodate other data types (e.g. PET) nipreps/nifreeze#21

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Make models inherit from base model #176

ENH: Make models inherit from base model #176

jhlegarreta commented Apr 16, 2024

jhlegarreta commented Apr 27, 2024

oesteban left a comment

jhlegarreta Apr 30, 2024

oesteban Jun 8, 2024

jhlegarreta Apr 30, 2024 •

edited

Loading

jhlegarreta May 27, 2024

jhlegarreta Apr 30, 2024

oesteban Jun 10, 2024

jhlegarreta Apr 30, 2024 •

edited

Loading

oesteban Jun 10, 2024

jhlegarreta commented Apr 30, 2024 •

edited

Loading

jhlegarreta commented May 15, 2024 •

edited

Loading

jhlegarreta May 27, 2024

jhlegarreta May 27, 2024 •

edited

Loading

jhlegarreta May 27, 2024

oesteban Jun 3, 2024

oesteban Jun 8, 2024

oesteban Jun 10, 2024

oesteban left a comment

oesteban Jun 3, 2024

oesteban commented Jun 7, 2024

oesteban left a comment

oesteban left a comment

oesteban Jun 8, 2024

oesteban Jun 8, 2024

jhlegarreta commented Jun 8, 2024

jhlegarreta commented Jun 12, 2024


		gradient = _rasb2dipy(gradient)
		self._gtab = _rasb2dipy(self._gtab)

	single_model = model.lower() in (
	"b0",
	"s0",
	"avg",
	"average",
	"mean",
	) or model.lower().startswith("full")

	dwmodel = None
	if single_model:
	if model.lower().startswith("full"):
	model = model[4:]

	# Factory creates the appropriate model and pipes arguments
	dwmodel = ModelFactory.init(
	model=model,
	**kwargs,
	)
	dwmodel.fit(dwdata.dataobj, n_jobs=n_jobs)

ENH: Make models inherit from base model #176

ENH: Make models inherit from base model #176

Conversation

jhlegarreta commented Apr 16, 2024

jhlegarreta commented Apr 27, 2024

oesteban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhlegarreta Apr 30, 2024 • edited Loading

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

jhlegarreta Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhlegarreta commented Apr 30, 2024 • edited Loading

jhlegarreta commented May 15, 2024 • edited Loading

Choose a reason for hiding this comment

jhlegarreta May 27, 2024 • edited Loading

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

oesteban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oesteban commented Jun 7, 2024

oesteban left a comment

Choose a reason for hiding this comment

oesteban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhlegarreta commented Jun 8, 2024

jhlegarreta commented Jun 12, 2024

jhlegarreta Apr 30, 2024 •

edited

Loading

jhlegarreta Apr 30, 2024 •

edited

Loading

jhlegarreta commented Apr 30, 2024 •

edited

Loading

jhlegarreta commented May 15, 2024 •

edited

Loading

jhlegarreta May 27, 2024 •

edited

Loading