search

theislab / cellrank

CellRank: dynamics from multi-view single-cell data
https://cellrank.org
BSD 3-Clause "New" or "Revised" License
341 stars 47 forks source link

`estimator.fit()` should compute a Schur decomposition #910

Closed Marius1311 closed 2 years ago

Marius1311 commented 2 years ago

I think it makes a lot of sense to have the .fit() method computing a Schur decomposition; this is one of the steps in the pipeline that should require the least amount of user control so it would be convenient to have it done in one single computation. What's your opinion @michalk8 ?

Marius1311 commented 2 years ago

I would also change the default number of Schur components to be 20; this won't make a difference in brandts mode as we're computing a full decomposition either, and it should really matter in krylov mode either. My motivation is that 10 is a bit low for the number of macrostates, I think realistic numbers are between 5 and 15 so we should set the default number of schur components accordingly.

michalk8 commented 2 years ago

I think it makes a lot of sense to have the .fit() method computing a Schur decomposition; this is one of the steps in the pipeline that should require the least amount of user control so it would be convenient to have it done in one single computation. What's your opinion @michalk8 ?

This is already being done.

I would also change the default number of Schur components to be 20; this won't make a difference in brandts mode as we're computing a full decomposition either, and it should really matter in krylov mode either. My motivation is that 10 is a bit low for the number of macrostates, I think realistic numbers are between 5 and 15 so we should set the default number of schur components accordingly.

Ok, agreed.

Marius1311 commented 2 years ago

Error only occurs when calling the .fit() method with a range for n_states.

Marius1311 commented 2 years ago

Great, thanks @michalk8