-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Comments
Maybe related to #3510 ? |
Why does Is this the desired behaviour: https://codepen.io/etpinard/pen/qgGxaW?editors=0010 |
it doesn't need one.
Yes. But if you're using a template which sets automargins (which plotly.py and Chart Studio do by default) then you get |
Cool. On it! |
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. |
By this same bug, you mean the page hangs? |
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 Although it doesn't happen to everyone. One colleague of mine can reproduce it with the same codepen, another reproduces it with this one |
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? |
I'm @destradafilm 's coworker who can also reproduce it with 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
with the condition that ` fullLayout._redrawFromAutoMarginCount > 100 The callstack I see is :
Where the last (top) 4 lines recurse until it fails |
@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: 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 |
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!? |
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. |
@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. |
Here's an attempt at fixing this bug: https://github.com/plotly/plotly.js/compare/better-did-margin-change-check Could you try out bundles:
Thanks very much! |
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! |
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
Ok well, that's great news. I'll wait for confirmation before making a PR. Thank you very much again for your help! |
@etpinard I can confirm that the bundle:
works! |
... 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)
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
The text was updated successfully, but these errors were encountered: