Skip to content

Conversation

@emmanuelle
Copy link
Contributor

I'm planning to complete the docstring and add examples in the tutorials, but I would like first to discuss whether this xarray support should rather go here in imshow or/and in a px.matrix_heatmap / px.heatmap function which is yet to be written (but could be adapted from the imshow code). @nicolaskruchten interested in your thoughts here :-).

@emmanuelle
Copy link
Contributor Author

Oh and I will fix the CI too of course (add xarray to CI deps). My hesitation here is in particular for arrays generated by datashader: since their dimensions are sometimes heterogeneous, it does not always make sense to plot them with square pixels.

@emmanuelle emmanuelle changed the title [WIP] xarray support in imshow xarray support in imshow Feb 10, 2020
@nicolaskruchten
Copy link
Contributor

This looks great, but per semver rules belongs in 4.6 :)

@emmanuelle
Copy link
Contributor Author

no worries :-). Anything to change ?

@emmanuelle
Copy link
Contributor Author

Now that the patch release is out, I think this one is ready for review :-).

layout_patch["margin"] = {"t": 60}
fig = go.Figure(data=trace, layout=layout)
fig.update_layout(layout_patch)
if img_is_xarray:
Copy link
Contributor

@nicolaskruchten nicolaskruchten Mar 20, 2020

Choose a reason for hiding this comment

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

I actually think we can do this for np inputs also, no, given the labels dict? We can just default x_label to x etc

@nicolaskruchten
Copy link
Contributor

@emmanuelle LMK what you think of my changes... missing additional tests, docstring and docs but I will add them if you agree.

Basically what I've done is generalized x and y a little bit so that a user can pass them in even with a normal numpy array for img. In essence, an alternative way to look at this PR with my changes is that it first adds user-controllable x, y and labels to imshow(), and then adds special handling for xarrays to prepopulate those values if unspecified ;)

@emmanuelle
Copy link
Contributor Author

Thank you this looks great ! Looks like you added the tests already, 💃 !

@nicolaskruchten nicolaskruchten merged commit 60e3666 into master Mar 23, 2020
@emmanuelle emmanuelle deleted the imshow-xarray branch March 27, 2020 16:27
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.

3 participants