Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to formulaic-contrasts #344

Merged
merged 5 commits into from
Dec 3, 2024
Merged

Switch to formulaic-contrasts #344

merged 5 commits into from
Dec 3, 2024

Conversation

grst
Copy link
Contributor

@grst grst commented Dec 1, 2024

Hi @BorisMuzellec,

I created formulaic-contrasts containing all the code required for introspecting formulaic models and defining contrasts.

As described here it can be used either by inheritance or composition. Using inheritance would save us from redefining .variables, .cond and .contrast, but there'd be a name clash with the design variable so I went for the composition approach instead. Using inheritance/mixin would be possible with a little bit of refactoring, but I'd leave that to you should you prefer this approach.

There's now also documentation on how to build contrasts: https://formulaic-contrasts.readthedocs.io/en/latest/contrasts.html
Feel free to reference that from and/or copy it to the PyDESeq2 docs.

Best,
Gregor

@BorisMuzellec
Copy link
Collaborator

Hi @grst,

Thanks for creating this PR! I'm happy with the way you propose to reorganize things :)

I'll add a reference to https://formulaic-contrasts.readthedocs.io/en/latest/contrasts.html in the docs when I'll have some time, let me open an issue to remember to do it

@BorisMuzellec BorisMuzellec marked this pull request as ready for review December 3, 2024 15:19
@BorisMuzellec BorisMuzellec merged commit 1d368bd into owkin:main Dec 3, 2024
5 checks passed
@@ -146,7 +146,7 @@ def test_deseq_independent_filtering_mean_fit(counts_df, metadata, tol=0.02):
)
dds.deseq2()

ds = DeseqStats(dds, contrast=["condition", "A", "B"])
ds = DeseqStats(dds, contrast=["condition", "B", "A"])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hang on, is this what you expect? it should be [variable, baseline, group_to_compare], right? I just want to make sure I didn't mix up things in formulaic-contrasts

Copy link
Collaborator

@BorisMuzellec BorisMuzellec Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be [variable, group_to_compare, baseline], and I think that you are right and that I got confused in #328

It used to be optional to input a contrast to DeseqStats before #328. I went a bit too fast in #328 and put the contrasts that passed the tests, but I realised that the code used to be

def _contrast(self, column: str, baseline: str, group_to_compare: str) -> np.ndarray:
    """Build a simple contrast for pairwise comparisons.
    This is equivalent to
    ```
    model.cond(<column> = baseline) - model.cond(<column> = group_to_compare)
    ```
    Parameters
    ----------
    column: str
        The column to contrast.
    baseline: str
        The baseline group.
    group_to_compare: str
        The group to compare to the baseline.
    Returns
    -------
    np.ndarray
        The contrast vector.
    """
    return self.dds.cond(**{column: baseline}) - self.dds.cond(
        **{column: group_to_compare}
    )

Which (unless I'm mistaken again!) was wrong (it should be
self.dds.cond(**{column: group_to_compare}) - self.dds.cond(**{column: baseline}))

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants