Conversation
maikia
left a comment
maikia
left a comment
There was a problem hiding this comment.
nice work @BorisMuzellec
I think it might be useful to start doing unit test to separate functions (maybe not here as it's already a large PR), ie not just compare the results to R but test for different inputs to the new function. It will be much easier to debug once error appears and you can test for more edge cases this way
pydeseq2/DeseqDataSet.py
Outdated
| self.design_matrix = build_design_matrix( | ||
| self.clinical, | ||
| design_factor, | ||
| design_factors, |
There was a problem hiding this comment.
just a minor comment: I would suggest to add the argument names which adds for the clarity of read (not required for merge though)
There was a problem hiding this comment.
Hi Maria, not sure what you mean, do you mean we should write design=design_factors in the arguments?
There was a problem hiding this comment.
yes, that's what I meant. Thanks :-)
| with parallel_backend("loky", inner_max_num_threads=1): | ||
| mu_hat_ = np.array( | ||
| Parallel( | ||
| # mu_hat is initialized differently depending on the number of different factor |
There was a problem hiding this comment.
do you think it might be useful to through a warning or otherwise give the comment to the user on which fit was used? Or at least add it to the docstring so it is easier to find? (please ignore if you don't think it is useful information to the user)
There was a problem hiding this comment.
That's a good question. Personally I don't think a warning is necessary, as the _mu_hat attribute is not meant to be accessed by the user (it's an intermediate value). DESeq2 does this silently too. If that's OK for you, I'll leave this as is for now.
| dds : DeseqDataSet | ||
| DeseqDataSet for which dispersion and LFCs were already estimated. | ||
|
|
||
| contrast : list[str] or None |
There was a problem hiding this comment.
from this description I am not sure what contrast actually is (just the format of it) and when to use it instead of None. Could you add a short description?
There was a problem hiding this comment.
I've expanded the description of this argument : 8c2197a
Let me know if this helps, or if it is still a bit obscure
| # Build contrast if None | ||
| if contrast is not None: | ||
| # TODO : tests on the contrast | ||
| self.contrast = contrast |
There was a problem hiding this comment.
maybe check if the contrast is indeed given as required? ie three strings in a list and through an error otherwise
pydeseq2/DeseqStats.py
Outdated
| offset, | ||
| prior_no_shrink_scale, | ||
| prior_scale, | ||
| "L-BFGS-B", |
There was a problem hiding this comment.
maybe add a comment that it's an optimizer
There was a problem hiding this comment.
I've added the argument name ("optimizer") in 3c0ded6
Co-authored-by: Maria Telenczuk <maria.telenczuk@owkin.com>
…iable names when calling functions
|
Thanks a lot @maikia for your review, it was super helpful :). |
maikia
left a comment
maikia
left a comment
There was a problem hiding this comment.
All good to go, thanks for your updates @BorisMuzellec
2 participants
This (big) PR aims to implement paired multi-factor analysis.
It is now possible to provide several design factors to a
DeseqDataSet, as long as they are bi-level.TODO: an example notebook (in another PR ?)