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

image trace type #4289

Merged
merged 16 commits into from
Oct 24, 2019
Merged

image trace type #4289

merged 16 commits into from
Oct 24, 2019

Conversation

antoinerg
Copy link
Contributor

@antoinerg antoinerg commented Oct 21, 2019

This PR introduces a new image trace type to display images on cartesian axes and closes #3898

Codepen showing the different mocks: https://codepen.io/antoinerg/pen/OJJPxJK

Quick overview:

  • Support different colormodels (rgb, rgba, hsl and hsla).
  • Can rescale data via zmin and zmax
  • Display data and color information on hover

Todo:

  • handle z array with missing values (ie. columns with shorter length) (done in 15c06fa)
  • support categorial axes
  • support log axis

To discuss:

  • the trace will be name image ~~decide the trace's name. ~~ Scroll down below to read different points of view on the matter. Here's a compiled a list of candidates:
    1. image
    2. raster
    3. rasterimage
    4. pixmap
    5. imshow
  • decide on the default hover label. Display the color or not?

@etpinard

This comment has been minimized.

@antoinerg
Copy link
Contributor Author

@etpinard It's ready for a first round of review !

@etpinard etpinard added this to the v1.51.0 milestone Oct 21, 2019
lib/index.js Outdated Show resolved Hide resolved
'For example, for the `rgba` colormodel, the default value is [255, 255, 255, 1].'
].join(' ')
},
x0: {
Copy link
Contributor

Choose a reason for hiding this comment

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

So no x and y for this iteration? Is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know, this wasn't a requirement for that iteration so I did not include it. For your information, most other libraries do not support this either. It is usually assumed that an image is made of pixels of identical size arranged in a regular fashion as in a square crystal lattice. I'd be surprised if users ask for x and y but we'll see 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

x0 and dx seem enough for a v1... any obvious cases where that wouldn't be enough? @jonmmease @emmanuelle @alexcjohnson ?

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I'm also very much ok with not implementing x and y in this iteration.

src/traces/image/calc.js Outdated Show resolved Hide resolved
src/traces/image/calc.js Outdated Show resolved Hide resolved
src/traces/image/hover.js Outdated Show resolved Hide resolved
src/traces/image/hover.js Outdated Show resolved Hide resolved
src/traces/image/plot.js Outdated Show resolved Hide resolved
@etpinard
Copy link
Contributor

Very nicely done @antoinerg 🎉 - I think you found the perfect way to get this new trace type out fast with solid performance and not much brittle logic. Thank you very much for that!

I made a few comments - none of them should be difficult to resolve. I'll add one more comment here:

  • I'd still vote for naming this trace raster. I'm not a fan of having a the term "image" refer to things in the layout AND the data.

@etpinard
Copy link
Contributor

Oh and one more thing @antoinerg: I didn't notice any funky CSS tricks like you had in one of your proof-of-concepts. I guess you decide to not go down that route, can you confirm?

@antoinerg
Copy link
Contributor Author

Oh and one more thing @antoinerg: I didn't notice any funky CSS tricks like you had in one of your proof-of-concepts. I guess you decide to not go down that route, can you confirm?

I confirm that I ⚡ the CSS trick I had in my proof of concept. If you want me to, I could add a few comments/references about CSS image-rendering: pixelated which could potentially be used in the future to speed up rendering on browsers that support it (almost all of them except Safari and potentially SVG editors like Illustrator/Inkscape).

I decided not to go that route because it's a bit unsafe and would require manually testing all the different browsers we want to support...

@jackparmer

This comment has been minimized.

@emmanuelle

This comment has been minimized.

@emmanuelle

This comment has been minimized.

@emmanuelle

This comment has been minimized.

@jackparmer

This comment has been minimized.

@archmoj

This comment has been minimized.

@YoniChechik

This comment has been minimized.

@emmanuelle

This comment has been minimized.

@emmanuelle

This comment has been minimized.

@etpinard

This comment has been minimized.

@antoinerg

This comment has been minimized.

@etpinard
Copy link
Contributor

Great, all my comments have been resolved. Awesome work @antoinerg !!

PSA: we're picking image (not raster, not pixmap, not anything else). Turns out I was the only one that didn't like image.

💃 💃 💃

@antoinerg
Copy link
Contributor Author

PSA: we're picking image (not raster, not pixmap, not anything else). Turns out I was the only one that didn't like image.

I am with you @etpinard: I'm not a big fan of image either and would have preferred pixmap. It seems like image is more popular so let's give the people what they want!

@antoinerg antoinerg merged commit 99fc02f into master Oct 24, 2019
@antoinerg antoinerg deleted the pr-image branch October 24, 2019 15:36
@archmoj
Copy link
Contributor

archmoj commented Oct 24, 2019

PSA: we're picking image (not raster, not pixmap, not anything else). Turns out I was the only one that didn't like image.

Me too!

@pfeatherstone
Copy link

Fantastic stuff. Any plans of updating the docs? At the moment i'm using the demo for guidance

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.

raster trace type: matplotlib imshow equivalent
8 participants