Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Px special inputs #2330

Closed
wants to merge 4 commits into from
Closed

Px special inputs #2330

wants to merge 4 commits into from

Conversation

nicolaskruchten
Copy link
Contributor

Closes #2119 ... @emmanuelle LMK what you think of the API and testing approach and if it makes sense I'll document it.

@@ -41,6 +41,30 @@ def __init__(self):
defaults = PxDefaults()
del PxDefaults


class IdentityMap(object):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could these two classes be moved to a _special_inputs.py file to shorten a bit _core.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, if needed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as you wish, I just feel that the length of this file makes it a bit overwhelming.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

@emmanuelle
Copy link
Contributor

Thanks for the PR! I'm completely convinced by px.Constant, a bit less by px.IdentityMap which looks like a "dummy" class (it does not do much from what I understand of the code and tests). I'm curious to know why you chose a class instead of the 'identity' string which we had mentioned.

I'll do more QA on examples tomorrow, tonight I only had time for a quick pass reading the code.

@nicolaskruchten
Copy link
Contributor Author

So IdentityMap isn't a dummy class, it's the actual implementation: each IdentityMap object returns the key on lookup, which is what we want in this case. I didn't implement the "identity" string thing yet just to see what this would feel like. I actually like this, personally, as it's a bit more Pythonic in a way, and doesn't have a "magic string" variable in a place where otherwise only a dict-like object is accepted. It's pretty trivial to implement the "identity" thing as well, just an if in make_mapping() somwhere to force val_map to be an IdentityMap when that string is encountered.

@emmanuelle
Copy link
Contributor

Ah ok thanks for the explanations, I had not understood how the class worked. Maybe add a line with your explanation above in the docstring of the class?

@emmanuelle
Copy link
Contributor

So I've experimented a bit with your branch. It works well as expected. From a pure usability point of view, doing something like fig = px.scatter(x=[1, 2], y=[3, 4], color=px.IdentityMap(['yellow', 'blue']) would be slightly nicer than fig = px.scatter(x=[1, 2], y=[3, 4], color=['yellow', 'blue'], color_discrete_map=px.IdentityMap()) but on the other hand the current implementation is simple and elegant (so simple I had originally thought that something was missing!). Thoughts?

@nicolaskruchten
Copy link
Contributor Author

nicolaskruchten commented Mar 30, 2020

Yep, fig = px.scatter(x=[1, 2], y=[3, 4], color=px.IdentityMap(['yellow', 'blue']) was one of the options I had listed in #2119 (as px.Identity) but after our conversation I thought that the second option seemed better...

@nicolaskruchten
Copy link
Contributor Author

OK I addressed your feedback, including the "identity" shortcut.

@emmanuelle
Copy link
Contributor

Yep, fig = px.scatter(x=[1, 2], y=[3, 4], color=px.IdentityMap(['yellow', 'blue']) was one of the options I had listed in #2119 (as px.Direct) but after our conversation I thought that the second option seemed better...

Oops... reading again our convo in #2119 I think I had preferred option 2 because it used a constant instead of a function or a class, but eventually it's not a constant...

@nicolaskruchten
Copy link
Contributor Author

I had preferred option 2 because it used a constant instead of a function or a class, but eventually it's not a constant...

I added this in: you can use "identity" now also (under the hood it just swaps out an IdentityMap for you so you don't have to see it).

I can implement px.Identity and px.Direct still if you think it will help?

@emmanuelle
Copy link
Contributor

Thanks for implementing the 'identity' thing. px.Direct could be useful indeed - I personally prefer using fig.update_traces but I understand that a fraction of users will want to do everything from px. As for px.Identity this is less necessary in my opinion, since when there are more than one colour people they will often exist already as a column in a DataFrame so people might as well do color='my_color_column', color_discrete_map="identity".

px.Direct can also come in another PR so feel free to merge this when you want. About docs: do you plan to add docs as part of this PR or together with one of the WIP PRs?

@nicolaskruchten
Copy link
Contributor Author

I was going to document everything in the last PR :)

@nicolaskruchten
Copy link
Contributor Author

Superseded by #2336

@nicolaskruchten nicolaskruchten deleted the px_special_inputs branch June 19, 2020 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

px.Constant and the "identity" problem
2 participants