Less hacky plotting classes #3209
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3209 +/- ##
==========================================
- Coverage 76.85% 76.69% -0.16%
==========================================
Files 109 109
Lines 12554 12616 +62
==========================================
+ Hits 9648 9676 +28
- Misses 2906 2940 +34
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Instead of explicit old all-caps defaults, could be possible it to make a lookup of properties -> old all-caps defaults? And then have some sort of __get_attr__ or __get_attribute__ do the lookup and use the default? This might also make typing a bit easier maybe? Not sure though, just a quick first pass
| def vboundnorm(self) -> VBoundNorm: | ||
| return VBoundNorm(self.vmin, self.vmax, self.vcenter, self.norm) | ||
|
|
||
| # deprecated class vars |
There was a problem hiding this comment.
Why deprecated? Because they are already set from elsewhere (the function-based API)?
There was a problem hiding this comment.
If so, why re-introduce?
There was a problem hiding this comment.
So these are part of the public API, the idea being to get rid of these eventually? https://scanpy.readthedocs.io/en/stable/api/generated/classes/scanpy.pl.StackedViolin.html
There was a problem hiding this comment.
Yeah, the default would be accessible like default = Baseplot.cmap, and overridable in subclasses:
class DotPlot(BasePlot):
cmap = "Blues"or just by setting it on the instance.
| # TODO: this doesn’t make much sense | ||
| self.category_height, self.category_width = ( | ||
| self.category_width, | ||
| self.category_height, | ||
| ) |
There was a problem hiding this comment.
Agreed needs some investigating (what's the counter-factual when we remove it?)
| norm=norm, | ||
| **kwds, | ||
| ) | ||
| DEFAULT_DOT_MAX: ClassVar[DefaultProxy[float | None]] = DefaultProxy("dot_max") |
There was a problem hiding this comment.
Unfortunate that the types need to be duplicated i.e., dot_max: float | None and then here DEFAULT_DOT_MAX: ClassVar[DefaultProxy[float | None]]
There was a problem hiding this comment.
Maybe worth creating type variables? Seems like that is more lines of code, though...
An attempt to make the plotting classes less weird
contains a descriptor to allow the DEFAULT_BLAH class attributes to still work.
@ilan-gold please take a preliminary look