removed colors argument for Table#scatter #389
Conversation
| # TODO: In a future release, warn that this is deprecated. | ||
| # Deprecated | ||
| # warnings.warn("scatter(colors=x) is deprecated. Use scatter(group=x)", FutureWarning) | ||
| if "colors" in vargs and vargs["colors"]: |
There was a problem hiding this comment.
Now that I think about it, this should warn on run of the code instead of just the first run. Thoughts?
There was a problem hiding this comment.
Agreed. Seems like warning every time might be preferable.
|
The previous handling of colors for scatter was rather broken because of
internal handling through a shared wrapper. If you specified the kwarg it
errred. So no existing code is passing the colors= kwarg in. But lots of
code wishes it could. The old use of colors was to provide difference
colors for different columns in the scatter. That behavior is reasonable
when no color arg is provided. The other (more typical) use is to use
distinct colors for distinct elements in a (y) column. A very natural
idiom would be to have a "color column" or array. The challenge here is if
one wants different coloring for each of more than one y-column in the
table. We currently have the y-columns as the set of non-x columns. The
color kwarg should be conformant with that. With a single y-column it
could be an array. More generally it could be a sequence of column labels,
which would be removed form the y-column set and utilized in order to color
the corresponding columns. Slight generalization would be to also accept a
"color table" that would be conformant with the y-columns of the scatter.
David E. Culler
Interim Dean of Data Sciences
Friesen Professor of Electrical Engineering and Computer Sciences
University of California, Berkeley
…On Fri, Jun 28, 2019 at 5:26 AM Adnan Hemani ***@***.***> wrote:
[N/A] Wrote test for feature
[X] Added changes in the Changelog section in README.md
[X] Bumped version number (delete if unneeded)
*Changes proposed:*
The next commit to remove support for the colors argument altogether and
warn the user to use group instead.
------------------------------
You can view, comment on, or merge this pull request online at:
#389
Commit Summary
- removed colors argument for Table#scatter
File Changes
- *M* README.md
<https://github.com/data-8/datascience/pull/389/files#diff-0> (3)
- *M* datascience/tables.py
<https://github.com/data-8/datascience/pull/389/files#diff-1> (13)
- *M* datascience/version.py
<https://github.com/data-8/datascience/pull/389/files#diff-2> (2)
Patch Links:
- https://github.com/data-8/datascience/pull/389.patch
- https://github.com/data-8/datascience/pull/389.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#389?email_source=notifications&email_token=ABSX42RTGKDFXVE6F66YN63P4X7N7A5CNFSM4H4E5GD2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4G4JW3RA>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABSX42S5ME6SKKCQ5RE6WZ3P4X7N7ANCNFSM4H4E5GDQ>
.
|
|
@adnanhemani, Right now we accept either |
|
@deculler, we do have many instances of Your idea about supporting choosing a different color for each point with a colors column is a neat one. That sounds like something we can handle as a separate feature request. Want to file an issue for that feature request so we can keep track of it? We'll probably prioritize based on whether we have anyone who wants to use it. |
|
That is what the colors= kwarg in matplotlib scatter does. Happy to do it
separately. Would have written the code a long time ago, but the internal
generation of a color= thru _draw was a pain to back out.
…On Fri, Jun 28, 2019 at 11:11 AM davidwagner ***@***.***> wrote:
@deculler <https://github.com/deculler>, we do have many instances of Table.scatter(...,
colors=...) in the Data 8 course materials: see #384 (comment)
<#384 (comment)>.
Your idea about supporting choosing a different color for each point with
a colors column is a neat one. That sounds like something we can handle as
a separate feature request. Want to file an issue for that feature request
so we can keep track of it? We'll probably prioritize based on whether we
have anyone who wants to use it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#389?email_source=notifications&email_token=ABSX42XC672D6RHTAV7FZKDP4ZH5DA5CNFSM4H4E5GD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY2ZRGA#issuecomment-506828952>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABSX42VBGFBBEEQRFSOZLUTP4ZH5DANCNFSM4H4E5GDQ>
.
|
|
@davidwagner maybe we should look into merging this through at some point? I think all the class materials were changed a while back to reflect the new method signature? |
[N/A] Wrote test for feature
[X] Added changes in the Changelog section in README.md
[X] Bumped version number (delete if unneeded)
Changes proposed:
The next commit to remove support for the colors argument altogether and warn the user to use group instead.