Fix/efp relative mode per group control from global control#179
Open
Fix/efp relative mode per group control from global control#179
Conversation
…e mode, had to add extra variables to include both absolute and relative
…rom-global-control
Collaborator
Author
|
@Yukthiw @bdls-jamal @mwkyuen can you take a look? I got another PR on top of this, want to keep things up to date. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
So there was an issue with colouring (both absolute [ fixed in #174 ] and now in relative [this fix]). Basically had to:
Problem
The eFP relative color mode and log2 fold change tooltip were using data.control — the average of all group controls across the entire experiment — instead of each tissue's own group.control. This produced incorrect colours and values in any experiment where groups have different controls.
Summary of Fixes:
Before (Note that even the control samples are non-zero [they should always be 0 since its log 2 fold change of 1]):

After:

Details:
Commit 1 — per-group control for relative mode
EFPTooltip.tsx: tooltip log2 fold change now uses group.control (falls back to data.control, then 1)
svg.tsx: useStyles passes group instead of data to getColor, so the relative-mode extremum calculation scales against the group's own min/max and control
legend.tsx: removed unused EFPState import; added a comment noting the legend is a global approximation that may not exactly match per-group colours
Commit 2 — preserve absolute mode scaling
The first commit broke absolute mode: passing group to getColor meant absolute colours normalised against group.max instead of the experiment-wide data.max, making every tissue appear fully red
Added an optional absoluteMax parameter to getColor; absolute mode now uses absoluteMax ?? group.max
useStyles passes data.max as the new argument
legend.tsx unchanged — it already passes data as its group arg, so the fallback resolves correctly
What was NOT changed
The XML parser — group-level tags are parsed correctly
The data shape in types.tsx or the loader in views/eFP/index.tsx — per-group control was already computed correctly; only downstream consumers were ignoring it, so we are just making sure it gets read
The data.control aggregate field — it remains as a fallback