Add back n_cpus argument in DeseqDataSet #226
Conversation
vcabeli
requested review from
BorisMuzellec,
maikia and
arthurPignetOwkin
as code owners
|
Thanks @vcabeli for this PR! Regarding the inference, IMO it's better not to require an |
pydeseq2/default_inference.py
Outdated
| self._n_processes = utils.get_num_processes(n_cpus) | ||
| self._backend = backend | ||
|
|
||
| @property | ||
| def n_cpus(self) -> int: # noqa: D102 | ||
| return self._n_processes | ||
|
|
||
| @n_cpus.setter | ||
| def n_cpus(self, n_cpus: int) -> None: | ||
| self._n_processes = n_cpus | ||
|
|
There was a problem hiding this comment.
I might be missing something but wouldn't it be simpler to just set self.n_cpus = utils.get_num_processes(n_cpus)?
There was a problem hiding this comment.
Right, without the abstract property it is indeed simpler to change the default inference's _n_processes to n_cpus
pydeseq2/dds.py
Outdated
| @@ -270,8 +278,20 @@ def __init__( | |||
| self.logmeans = None | |||
| self.filtered_genes = None | |||
|
|
|||
| if n_cpus: | |||
| n_cpus = get_num_processes(n_cpus) | |||
There was a problem hiding this comment.
I don't think we need to call get_num_processes here, because it's already called when the DefaultInference object is initialized. We could expect other inference implementations to handle the n_cpus argument themselves.
There was a problem hiding this comment.
We could expect other inference implementations to handle the n_cpus argument themselves
I think it's reasonable to expect a check during the __init__, but should we expect another check when setting the parameter ? When inference is specified, we would be overriding its n_cpus param without checking it first.
There was a problem hiding this comment.
We could add this check in the setter if we decide to keep it
BorisMuzellec
force-pushed
the
revert_n_cpus
branch
from
7b85cb6
to
775b678
Compare
Reference Issue or PRs
Fixes #214
What does your PR implement? Be specific.
Adding back n_cpus as an optional keyword argument in DeseqDataSet, and adding
n_cpusas an abstract property in the Inference class, in order to override the cpus of theinferenceinstance passed to DeseqDataSet (let me know if you prefer to not have that and just have a warning instead).