search

qiime2 / q2-diversity

BSD 3-Clause "New" or "Revised" License
4 stars 45 forks source link

ENH/API: support coloring Mantel scatters #318

Closed wasade closed 2 years ago

wasade commented 3 years ago

This pull request introduces a means to color the Mantel scatter plots by a categorical variable, which can help emphasize structure in the plot. An example is attached Screen Shot 2021-10-27 at 4 12 58 PM

ebolyen commented 3 years ago

That's a much prettier output! Also, the error you're getting confused me for a few minutes:

TypeError: Parameter 'metadata' received 'Metadata' as an argument, which is incompatible with parameter type: MetadataColumn[Categorical]

But it's complaining about a python string "Metadata" which is the default label for beta_correlation (label1). I think if you update that pipeline (probably with keywords) the tests should pass.

wasade commented 3 years ago

Okay cool, thanks for the quick check on that!

We need to be careful with how the coloring is communicated. And it's possible this should be backed out. The color is the sample on the X-axis, irrespective of what that sample is paired with for the distance. It may be better to limit to only same/same and shade the background?

ebolyen commented 3 years ago

That makes sense. In the future (not to feature-creep) perhaps the coloring could be dynamically set, which would make it super obvious that we are coloring one axis at a time. In the interim coloring only group_X:group_X points like you suggest would prevent any ambiguity.

lizgehret commented 2 years ago

Closed and re-opened to trigger CI workflow

lizgehret commented 2 years ago

That's a much prettier output! Also, the error you're getting confused me for a few minutes:

TypeError: Parameter 'metadata' received 'Metadata' as an argument, which is incompatible with parameter type: MetadataColumn[Categorical]

But it's complaining about a python string "Metadata" which is the default label for beta_correlation (label1). I think if you update that pipeline (probably with keywords) the tests should pass.

Hey @wasade I took a quick look over this PR and re-ran ci and we're still getting the same test failures that @ebolyen mentioned above. Our PR cutoff for the 2022.8 release was technically yesterday, so unless you have something ready to go here to address these failures before EOD we'll have to hold off on this one until the 2022.11 release.

wasade commented 2 years ago

TBH its been so long I don't recall what this really does. I'm on leave for a few more weeks, it would be okay to close this.

On Fri, Aug 19, 2022, 14:01 Liz Gehret @.***> wrote:

That's a much prettier output! Also, the error you're getting confused me for a few minutes:

TypeError: Parameter 'metadata' received 'Metadata' as an argument, which is incompatible with parameter type: MetadataColumn[Categorical]

But it's complaining about a python string "Metadata" which is the default label for beta_correlation (label1). I think if you update that pipeline (probably with keywords) the tests should pass.

Hey @wasade https://github.com/wasade I took a quick look over this PR and re-ran ci and we're still getting the same test failures that @ebolyen https://github.com/ebolyen mentioned above. Our PR cutoff for the 2022.8 release was technically yesterday, so unless you have something ready to go here to address these failures before EOD we'll have to hold off on this one until the 2022.11 release.

— Reply to this email directly, view it on GitHub https://github.com/qiime2/q2-diversity/pull/318#issuecomment-1221089159, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADTZMV7LKXKJAJH2OLSSBLVZ7YY5ANCNFSM5G3PX5DA . You are receiving this because you were mentioned.Message ID: @.***>

lizgehret commented 2 years ago

Sounds good @wasade, I'll go ahead and close it! We can always re-open if/when you have bandwidth and want to fix up these test failures. Enjoy your time off! 🙂