ENH: Adding in invert option #83

mortonjt · 2019-08-23T20:05:45Z

This PR accomplishes 2 things

Renames samples/features -> points/arrows
Adds an inversion flag to allow for points to be swapped with arrows.

This resolves the following issues

qiime2/q2-diversity#258
biocore/emperor#732
#82

See the following screenshot

CC @ElDeveloper @thermokarst @nbokulich

…ures with points/arrows

ElDeveloper · 2019-08-25T21:23:59Z

This looks good to me overall, although I would be interested to hear from @thermokarst, @nbokulich and @ebolyen on how we should proceed with this backwards incompatible change. Is there a preferred method to deprecate/change user-facing things like these?

ebolyen · 2019-08-26T18:11:55Z

So we don't have a good mechanism to handle parameter deprecation (yet!). So avoiding that may be best for now. (FWIW, the rename here is incomplete as number_of_features is now "vocabularistically" stranded.)

We also have an transpose action for feature tables now, so I think inverting samples/feature isn't the worst thing, but I totally get the motivation behind this rename, and in the future I am all for it. In the meanwhile having just invert seems like enough to me.

mortonjt · 2019-08-29T18:36:14Z

Terms have been reverted, just the inversion option is added

mortonjt · 2019-10-02T17:45:46Z

Hi @ElDeveloper @ebolyen is there anything outstanding on this?

ElDeveloper · 2019-10-02T18:06:23Z

q2_emperor/_plot.py

+        features = biplot.features.copy()
+        samples = biplot.samples.copy()
+        biplot.samples = features
+        biplot.features = samples


This is one of those Pythonic patterns that can be done like:

biplot.samples, biplot.features = biplot.features, biplot.samples

ElDeveloper · 2019-10-02T18:29:11Z

Let's wait to see if @ebolyen can have a look.

mortonjt · 2019-10-02T18:30:12Z

q2_emperor/_plot.py

+        biplot.samples = features
+        biplot.features = samples


@ElDeveloper like this?

Suggested change

biplot.samples = features

biplot.features = samples

biplot.features, biplot.samples = samples, features

I think you can even bypass the intermediate assignment to samples and features.

mortonjt · 2019-10-02T18:30:43Z

q2_emperor/_plot.py

+    if invert:
+        features = biplot.features.copy()
+        samples = biplot.samples.copy()
+        biplot.samples = features


Suggested change

biplot.samples = features

mortonjt · 2019-10-02T18:42:18Z

I'm not following. Could you elaborate? GitHub just enabled suggested commits, which is pretty cool - you should be able to suggest the exact line change.

…

On Wed, Oct 2, 2019, 2:35 PM Yoshiki Vázquez Baeza ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In q2_emperor/_plot.py <#83 (comment)>: > number_of_features: int = 5) -> None: + if invert: + features = biplot.features.copy() + samples = biplot.samples.copy() + biplot.samples = features + biplot.features = samples I think you can even bypass the intermediate assignment to samples and features. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#83?email_source=notifications&email_token=AA75VXKAQZURDEZ5YNZQNADQMTSWFA5CNFSM4IPC46FKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCGVVTHI#discussion_r330706809>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA75VXOIAXLVMZHMVYYSZ4DQMTSWFANCNFSM4IPC46FA> .

ElDeveloper · 2019-10-02T18:48:45Z

Hi Jamie, I have tried the suggestions before, although you can't do multiline comments, hence why I wasn't able to do that. My suggestion was to change the code from: + if invert: + features = biplot.features.copy() + samples = biplot.samples.copy() + biplot.samples = features + biplot.features = samples into: + if invert: + biplot.samples, biplot.features = biplot.features, biplot.samples Does that make sense? Double assignments are a thing in python, and you don't need intermediate variables. Alternatively, am I missing something? Thanks! Yoshiki.

…

On (Oct-02-19|18:42), Jamie Morton wrote: I'm not following. Could you elaborate? GitHub just enabled suggested commits, which is pretty cool - you should be able to suggest the exact line change. On Wed, Oct 2, 2019, 2:35 PM Yoshiki Vázquez Baeza ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In q2_emperor/_plot.py > <#83 (comment)>: > > > number_of_features: int = 5) -> None: > > + if invert: > + features = biplot.features.copy() > + samples = biplot.samples.copy() > + biplot.samples = features > + biplot.features = samples > > I think you can even bypass the intermediate assignment to samples and > features. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#83?email_source=notifications&email_token=AA75VXKAQZURDEZ5YNZQNADQMTSWFA5CNFSM4IPC46FKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCGVVTHI#discussion_r330706809>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AA75VXOIAXLVMZHMVYYSZ4DQMTSWFANCNFSM4IPC46FA> > . > -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #83 (comment)

mortonjt · 2019-10-02T18:52:44Z

Oh, got it. I have some suggested changes in the PR - so merge those in when I'm back on the keyboard On Wed, Oct 2, 2019, 2:48 PM Yoshiki Vázquez Baeza <notifications@github.com> wrote:

…

Hi Jamie, I have tried the suggestions before, although you can't do multiline comments, hence why I wasn't able to do that. My suggestion was to change the code from: + if invert: + features = biplot.features.copy() + samples = biplot.samples.copy() + biplot.samples = features + biplot.features = samples into: + if invert: + biplot.samples, biplot.features = biplot.features, biplot.samples Does that make sense? Double assignments are a thing in python, and you don't need intermediate variables. Alternatively, am I missing something? Thanks! Yoshiki. On (Oct-02-19|18:42), Jamie Morton wrote: >I'm not following. Could you elaborate? >GitHub just enabled suggested commits, which is pretty cool - you should be >able to suggest the exact line change. > >On Wed, Oct 2, 2019, 2:35 PM Yoshiki Vázquez Baeza < ***@***.***> >wrote: > >> ***@***.**** commented on this pull request. >> ------------------------------ >> >> In q2_emperor/_plot.py >> <#83 (comment)>: >> >> > number_of_features: int = 5) -> None: >> >> + if invert: >> + features = biplot.features.copy() >> + samples = biplot.samples.copy() >> + biplot.samples = features >> + biplot.features = samples >> >> I think you can even bypass the intermediate assignment to samples and >> features. >> >> — >> You are receiving this because you authored the thread. >> Reply to this email directly, view it on GitHub >> < #83?email_source=notifications&email_token=AA75VXKAQZURDEZ5YNZQNADQMTSWFA5CNFSM4IPC46FKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCGVVTHI#discussion_r330706809 >, >> or mute the thread >> < https://github.com/notifications/unsubscribe-auth/AA75VXOIAXLVMZHMVYYSZ4DQMTSWFANCNFSM4IPC46FA > >> . >> > > >-- >You are receiving this because you were mentioned. >Reply to this email directly or view it on GitHub: >#83 (comment) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#83?email_source=notifications&email_token=AA75VXL7TB4LXV7YRIUSUX3QMTUI3A5CNFSM4IPC46FKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAFZEBA#issuecomment-537629188>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA75VXOQ3AB7MJKFGFYCOATQMTUI3ANCNFSM4IPC46FA> .

ebolyen

This looks good to me! Want me to push a commit with the assignment-swap?

ebolyen · 2019-10-12T00:08:23Z

Usability question:

Right now, using the invert flag forces us to swap the sample and feature metadata inputs, which isn't quite what I was expecting when I was testing the assignment-swap. Should we assume the orientation is consistent between the ordination and the metadata and it's only the plot that is being inverted, or does the inversion change the semantics of the other parameters?

I think arguments can be made both ways, but I kind of prefer just changing a single flag.

ElDeveloper · 2019-10-14T16:20:42Z

@ebolyen thanks for bringing this up. The error is particularly confusing since it always refers to samples even if the mismatch is in features. In a recent PR I made a few changes to make this type dependent i.e. by feature or sample. See this PR: biocore/emperor#734

…rtonjt-term-swap

ebolyen · 2019-11-08T15:50:59Z

Hey all, I played with this in the new emperor just before last release to see if the situation was any different, but same deal as before.

I think passing feature metadata to --m-sample-metadata-file is confusing (and it makes sense why the options were originally renamed). Would it be possible to leave the association between the arguments and parameters alone, and instead just flip the arrows/points internally?

mortonjt · 2019-11-08T16:38:14Z

yea - the naming scheme is rather unfortunate; changing the names to something more abstract (i.e. points / arrows) would be preferable.

@ebolyen what do you mean by flip the arrows / points internally? Would this nullify the use case for the invert option?

ebolyen · 2019-11-08T16:57:42Z

Sorry I mean that the samples are always samples and features are always features, but when you set invert you are choosing to draw the samples as arrows instead of points. So by internal, I mean from the outside nothing looks different (other than passing the single flag).

I think this makes more sense as now the arguments are always matched with their parameters, and you can just ask them to be drawn different from the default.

ebolyen · 2019-11-08T21:45:34Z

@mortonjt @ElDeveloper, does that last commit seem like a reasonable way to do this?

thermokarst · 2019-11-15T21:53:37Z

Pinging @mortonjt & @ElDeveloper (please see @ebolyen's request above)

ElDeveloper

This looks good to me!

ENH: Adding in invert option in addition to swapping the samples/feat…

f26000e

…ures with points/arrows

REV: reverting to sample/feature

16da043

mortonjt changed the title ~~ENH: Adding in invert option in addition to swapping the samples/feat…~~ ENH: Adding in invert option Aug 29, 2019

flake8

f27c502

ElDeveloper approved these changes Oct 2, 2019

View reviewed changes

mortonjt commented Oct 2, 2019

View reviewed changes

thermokarst assigned ebolyen Oct 2, 2019

thermokarst requested a review from ebolyen October 2, 2019 18:36

ebolyen assigned ebolyen and unassigned ebolyen Oct 10, 2019

ebolyen approved these changes Oct 10, 2019

View reviewed changes

SQUASH: @ElDeveloper's suggestion

5e4e874

Merge branch 'master' of https://github.com/qiime2/q2-emperor into mo…

00ea201

…rtonjt-term-swap

SQUASH: does this seem reasonable?

c050fcd

ebolyen removed their assignment Nov 8, 2019

ebolyen requested a review from ElDeveloper November 8, 2019 21:46

SQUASH: forgot to update test

122977c

thermokarst assigned ElDeveloper and mortonjt Nov 15, 2019

ElDeveloper approved these changes Nov 18, 2019

View reviewed changes

ebolyen merged commit 078e440 into qiime2:master Nov 18, 2019

This was referenced Jan 6, 2020

Enabling inversion of ordination objects biocore/emperor#732

Closed

Enabling inversion of points and arrows #82

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Adding in invert option #83

ENH: Adding in invert option #83

mortonjt commented Aug 23, 2019 •

edited

Loading

ElDeveloper commented Aug 25, 2019

ebolyen commented Aug 26, 2019

mortonjt commented Aug 29, 2019

mortonjt commented Oct 2, 2019

ElDeveloper Oct 2, 2019

ElDeveloper commented Oct 2, 2019

mortonjt Oct 2, 2019 •

edited

Loading

ElDeveloper Oct 2, 2019

mortonjt Oct 2, 2019

mortonjt commented Oct 2, 2019 via email

ElDeveloper commented Oct 2, 2019 via email

mortonjt commented Oct 2, 2019 via email

ebolyen left a comment

ebolyen commented Oct 12, 2019

ElDeveloper commented Oct 14, 2019

ebolyen commented Nov 8, 2019

mortonjt commented Nov 8, 2019

ebolyen commented Nov 8, 2019

ebolyen commented Nov 8, 2019

thermokarst commented Nov 15, 2019

ElDeveloper left a comment

	biplot.samples = features
	biplot.features = samples
	biplot.features, biplot.samples = samples, features

ENH: Adding in invert option #83

ENH: Adding in invert option #83

Conversation

mortonjt commented Aug 23, 2019 • edited Loading

ElDeveloper commented Aug 25, 2019

ebolyen commented Aug 26, 2019

mortonjt commented Aug 29, 2019

mortonjt commented Oct 2, 2019

ElDeveloper Oct 2, 2019

Choose a reason for hiding this comment

ElDeveloper commented Oct 2, 2019

mortonjt Oct 2, 2019 • edited Loading

Choose a reason for hiding this comment

ElDeveloper Oct 2, 2019

Choose a reason for hiding this comment

mortonjt Oct 2, 2019

Choose a reason for hiding this comment

mortonjt commented Oct 2, 2019 via email

ElDeveloper commented Oct 2, 2019 via email

mortonjt commented Oct 2, 2019 via email

ebolyen left a comment

Choose a reason for hiding this comment

ebolyen commented Oct 12, 2019

ElDeveloper commented Oct 14, 2019

ebolyen commented Nov 8, 2019

mortonjt commented Nov 8, 2019

ebolyen commented Nov 8, 2019

ebolyen commented Nov 8, 2019

thermokarst commented Nov 15, 2019

ElDeveloper left a comment

Choose a reason for hiding this comment

mortonjt commented Aug 23, 2019 •

edited

Loading

mortonjt Oct 2, 2019 •

edited

Loading