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

Automargin regression since 1.44.2 ? #3561

Closed
nicolaskruchten opened this issue Feb 21, 2019 · 17 comments · Fixed by #3566 or #3811
Closed

Automargin regression since 1.44.2 ? #3561

nicolaskruchten opened this issue Feb 21, 2019 · 17 comments · Fixed by #3566 or #3811
Assignees
Labels
bug something broken

Comments

@nicolaskruchten
Copy link
Contributor

See this pen: https://codepen.io/nicolaskruchten/pen/yZrdKO?editors=0010

It takes a long time to run (almost hangs in Chrome/OSX!) and then the solution is to add a bunch of padding at the bottom, which didn't happen on 1.44.2, as in this pen: https://codepen.io/nicolaskruchten/pen/QYPeWE

@nicolaskruchten
Copy link
Contributor Author

Maybe related to #3510 ?

@etpinard
Copy link
Contributor

Why does xaxis2 have a title in your codepen?

Is this the desired behaviour: https://codepen.io/etpinard/pen/qgGxaW?editors=0010

@nicolaskruchten
Copy link
Contributor Author

nicolaskruchten commented Feb 21, 2019

Why does xaxis2 have a title in your codepen?

it doesn't need one.

Is this the desired behaviour: https://codepen.io/etpinard/pen/qgGxaW?editors=0010

Yes. But if you're using a template which sets automargins (which plotly.py and Chart Studio do by default) then you get automargin: true on all axes by default. Setting automargins :true on both here even without the title makes the plot short again.

@etpinard
Copy link
Contributor

Cool. On it!

@destradafilm
Copy link

See this pen: https://codepen.io/nicolaskruchten/pen/yZrdKO?editors=0010

It takes a long time to run (almost hangs in Chrome/OSX!) and then the solution is to add a bunch of padding at the bottom, which didn't happen on 1.44.2, as in this pen: https://codepen.io/nicolaskruchten/pen/QYPeWE

I'm seeing this same bug with automargin and I'm on v1.46.0, I even tested in v1.47 and still get it too. I think this hasn't been fixed.

@etpinard
Copy link
Contributor

I'm seeing this same bug

By this same bug, you mean the page hangs?

@destradafilm
Copy link

yeah, I've been trying to recreate it but seems to need the perfect conditions to break. Even the original codepen submitted by @nicolaskruchten breaks sometimes for me.

can you test this one? it has a forced width / height (in pixels) to what the width / height in the original codepen (in vh) was rendered to on my browser.

https://codepen.io/destrada/pen/PgBroN?editors=1010

Eventually I get
plotly.min.js:1 Uncaught RangeError: Maximum call stack size exceeded

Screen Shot 2019-04-22 at 5 10 46 PM

Although it doesn't happen to everyone. One colleague of mine can reproduce it with the same codepen, another reproduces it with this one
https://codepen.io/destrada/pen/yrqdgj?editors=1011

@etpinard
Copy link
Contributor

OK thanks @destradafilm - I can't reproduce the problem from the two codepens you provided, but let me re-open the issue.

Can @plotly/plotly_js give:

a shot and see if a max call-stack errors pops up?

@etpinard etpinard reopened this Apr 22, 2019
@kevinoe
Copy link

kevinoe commented Apr 22, 2019

I'm @destradafilm 's coworker who can also reproduce it with
https://codepen.io/anon/pen/LvBooa?editors=0011

It seems to be very sensitive to geometry. You might try playing with pixel sizes in that pen. From what I can tell, plotly calculates automargins, replots, which then results in another call to calculate automargins, which gets slightly different results (ie a value switches from xx.4x to xx.5x and gets rounded differently), which replots, etc etc.

I put a conditional breakpoint in doAutoMargin below these lines:

    if(!fullLayout._replotting &&
        oldmargins !== '{}' &&
        oldmargins !== JSON.stringify(fullLayout._size)
    ) {
        if('_redrawFromAutoMarginCount' in fullLayout) {`

with the condition that ` fullLayout._redrawFromAutoMarginCount > 100

The callstack I see is :

plots.doAutoMargin (plots.js:1927)
lib.syncOrAsync (index.js:401)
exports.plot (plot_api.js:396)
exports.call (registry.js:224)
plots.doAutoMargin (plots.js:1927)
lib.syncOrAsync (index.js:401)
exports.plot (plot_api.js:396)
exports.newPlot (plot_api.js:666)
(anonymous) (pen.js:3)

Where the last (top) 4 lines recurse until it fails

@kevinoe
Copy link

kevinoe commented Apr 23, 2019

@etpinard - What sort of machine / os / browser are you using? We've seen a lot of variability in what triggers it.

For what its worth, I'm using:
MacBook Pro (13-inch, 2018, Four Thunderbolt 3 Ports)
OSX Mohave 10.14.4
Chrome Version 73.0.3683.103 (Official Build) (64-bit)

We have a fair variety of machines here - we might be able to try to tweak the code pen. Alternatively, I've created a new codepen (https://codepen.io/anon/pen/JVmPqB?editors=0011) which walks through a whole bunch of heights around the size I use. If you set the breakpoint I mentioned above, maybe you'll be able to reproduce

@kevinoe
Copy link

kevinoe commented Apr 25, 2019

One thing I've just noticed. I ran the above codepen (https://codepen.io/anon/pen/JVmPqB). On my machine I get the bug every time the vertical height it sets is divisible by 3!?

@etpinard
Copy link
Contributor

etpinard commented Apr 25, 2019

Thanks very much @kevinoe for diving deep into this!

Though I still haven't been able to reproduce this bug, it appears like we're dealing with some form of rounding errors. Adding some tolerance to our auto-margin checks should fix the problem.

@kevinoe
Copy link

kevinoe commented Apr 25, 2019

@etpinard - The newest code pen lets you scan various heights - that didn't do it for you? We're all on mac machines here - so wonder if that has something to do with it.

@etpinard
Copy link
Contributor

@kevinoe
Copy link

kevinoe commented Apr 25, 2019

Hi @etpinard - I tried it using the non-minified and that did NOT help. However, searching that file for "didMarginChange" fails, so perhaps that build is out of date? When I tried it with the minified version, I did not get the stack overflow. Going to see if my coworker @destradafilm can confirm using the minified version in his tests.

Thanks!

@etpinard
Copy link
Contributor

Ah oops, I pasted one link to a un-minified bundle (with a 0.5px tolerance) and another to minifed bundle (with 1px tolerance). Sorry for the confusion.

The link to the (bigger) 1px tolerence un-minified bundle is: https://34380-45646037-gh.circle-artifacts.com/0/dist/plotly.js

where didMarginChange is clearly there:

image

When I tried it with the minified version, I did not get the stack overflow. Going to see if my coworker @destradafilm can confirm using the minified version in his tests.

Ok well, that's great news. I'll wait for confirmation before making a PR.

Thank you very much again for your help!

@destradafilm
Copy link

destradafilm commented Apr 25, 2019

@etpinard I can confirm that the bundle:

The link to the (bigger) 1px tolerence un-minified bundle is: https://34380-45646037-gh.circle-artifacts.com/0/dist/plotly.js

works!

thanks @kevinoe and @etpinard !

etpinard added a commit that referenced this issue Apr 26, 2019
... to avoid potential infinite loops when rounding errors
    make auto-margins fail to converge.
    See report (that @etpinard was never able to reproduce) in
    #3561 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
4 participants