image: improve layout defaults

[antoinerg]

This PR adds a smart layout default of scaleanchor to make sure pixels are squares and constrain axes to the domain.

[archmoj]

@antoinerg thanks very much for the PR.
Kindly find my comments below.

[archmoj]

Curious to know how these numbers for the mock are achieved?
Did you read the raw data from a raw bitmap file (i.e. .bmp) using cpp?
Or decoded a .png with javascript or python? Or...

[etpinard]

@antoinerg what did you change in this (one-line) JSON file?

[antoinerg]

@etpinard I removed the scaleanchor that was initially present since it is now the default. This way, this mock also tests the smart default!

[antoinerg]

Curious to know how these numbers for the mock are achieved?
Did you read the raw data from a raw bitmap file (i.e. .bmp) using cpp?
Or decoded a .png with javascript or python? Or...

@archmoj I read the a png using the imread function in Python:

#! /usr/bin/env nix-shell
#! nix-shell -i python3 -p python3 python3Packages.scikitimage -p python3 python3Packages.numpy
import json
from skimage.io import imread
import numpy as np
# rgb_img = imread('small.jpg')

rgb_img = imread('/home/antoine/Downloads/Individual
Sprites/adventurer-idle-2-00.png')
# rgb_img[:, :, 3] = rgb_img[:, :, 3] / 255
figure = {
    "data": [{
        "z": rgb_img.tolist(),
        "x": 0.5,
        "y": 0.5,
        "colormodel": "rgba",
        "type": 'image'
    }],
    "layout": {
        "yaxis": {
            "scaleanchor": "x",
            "dtick": 1,
            "range": [-40, -5],
            "zeroline": False
        },
        "xaxis": {
            "dtick": 1,
            "zeroline": False
        },
        "width": 400,
        "height": 400,
        "margin": {"t": 0, "b": 0, "l": 0, "r": 0}
    }
}
json = json.dumps(figure)
print(json)

[etpinard]

null isn't a valid scaleanchor value. Please remove all the scalanchor: 'null' lines from the mocks. If you want to test that scaleanchor: null does the right thing, use a jasmine test.

[etpinard]

... this mean you'll have to update a few baselines, I hope you don't mind.

[antoinerg]

Done in d71c3b3

[etpinard]

Ok, thanks to the new baselines I realised that the constrain default is 'range'.

If I recall correctly, both @emmanuelle and @nicolaskruchten prefered the look of constrain: 'domain' for image traces.

Personally, I think constrain: 'range' looks just fine and it's probably not worth adding more smart (aka magic) defaults in this case.

Would it be ok to keep constrain: 'range' as default here?

[antoinerg]

Mmm 🤔 I'm not sure. I think constrain: 'domain' looks a bit better when a single image trace is present. I don't know how I feel about adding some more magic defaults. I'll let @emmanuelle and @nicolaskruchten chime in.

@etpinard About the smart defaults in image, where should we document them? I guess within the trace description in its index.js file?

[etpinard]

About the smart defaults in image, where should we document them? I guess within the trace description in its index.js file?

Yep, in the image/index.js description would be 👌

[antoinerg]

Done in 2b0d3c3

Let's wait on @emmanuelle and @nicolaskruchten about constrain: 'domain'

[antoinerg]

Sorry about that:

but I pushed a bad commit inadvertently. I didn't change any commits you've seen already.

[etpinard]

💃 for me - thanks very much @antoinerg

[antoinerg]

for me - thanks very much @antoinerg

Awesome! I'll wait a bit before merging this in case we want a smart layout default of constrain: 'domain'.

BTW, it is implemented in this branch. It's not super DRY but it is tested.

[emmanuelle]

for me - thanks very much @antoinerg

Awesome! I'll wait a bit before merging this in case we want a smart layout default of constrain: 'domain'.

BTW, it is implemented in this branch. It's not super DRY but it is tested.

Yes, constrain: 'domain' would look nicer (no background visible around the image)

[antoinerg]

Yes, constrain: 'domain' would look nicer (no background visible around the image)

Ok, I opened a PR against the current one: #4313

@etpinard can you review it?

[etpinard]

Yes, constrain: 'domain' would look nicer (no background visible around the image)

Ok maybe, but I'd argue it's much more magical.

@archmoj what do you think?

[archmoj]

Yes, constrain: 'domain' would look nicer (no background visible around the image)

Ok maybe, but I'd argue it's much more magical.

@archmoj what do you think?

I think the image trace is getting too magical when compared to other Cartesian traces.

[antoinerg]

I think the image trace is getting too magical when compared to other Cartesian traces.

I understand and share your sentiment. Now the question is where should that logic be? If it lives in plotly.py, then we'll also have to include it in our other wrappers (R and others) (ie. the logic will have to be duplicated downstream which is maybe not ideal).

Considering we already decided to automagically set scaleanchor and autorange, adding constrain to the list would be consistent with our current philosophy/efforts of providing better layout defaults for image. Now the real question is whether constrain: 'domain' really is better. If it is, I would be tempted to consider including image-scaleanchor...image-constrain

cc @nicolaskruchten @alexcjohnson @emmanuelle @archmoj

[nicolaskruchten]

I’m on the domain train.

[alexcjohnson]

To my mind there's not a whole lot of difference in magicality between turning on scaleanchor and that plus domain. If that's the right default user experience, let's do it. Honestly if we had implemented domain in the first iteration of constraints it would probably be the default anyway.

(I haven't looked at that other branch @antoinerg but I hope domain is set on both axes...)

[etpinard]

Ooof, I think I need a vacation.

[etpinard]

@antoinerg sorry to make you do this, but I found a compromise with @alexcjohnson here's a screenshot of our convo:

---

So this means,

• domain constraints are going in
• but we'll change the on/off logic: any subplot that has one or more image trace should get a scaleanchor and a domain constraint (regardless of what other trace types there are on that subplot).

---

If you need help fixing all this, please let me know. I can help out

[archmoj]

• but we'll change the on/off logic: any subplot that has one or more image trace should get a scaleanchor and a domain constraint (regardless of what other trace types there are on that subplot).

And what about image reversing the Y-axis? It would make sense to keep the axis reversed when other traces are added on top.

[etpinard]

And what about image reversing the Y-axis? I

Good call! Yes, that too. Every image-induced axis default should be turned on on every subplot containing one or more image trace.

[nicolaskruchten]

That all sounds great to me.

[antoinerg]

I'm glad we reached a consensus! I'd like to thank everybody for their valuable input.

@etpinard I went ahead and made the changes described in #4307 (comment) in PR #4313 which we would merge in the current PR. Let me know if that is 👌

[etpinard]

should say should reverse yaxis if images are present i.e. no only

[antoinerg]

Well, that particular test only has images. We could probably 🔪 it considering we test image WITH other traces in the next test.

[etpinard]

I'd just 🔪 the only. There's a nice progression in complexity in this declare block

[antoinerg]

Sweet! Done in 408b46b

[etpinard]

similarly here, 🔪 that only

[antoinerg]

Should I 🔪 the whole test instead?

[antoinerg]

Done in 408b46b

[etpinard]

🔪 only

[antoinerg]

Again, should I 🔪 the whole test?

[antoinerg]

Done in 408b46b

[etpinard]

My comments about the only strings are non-blocking

💃 - thanks very much @antoinerg for the quick turnaround!!

[archmoj]

_ 1 _ 2
1:dancer: :dancer:
2:dancer: :dancer: