Repository navigation
MNT: Re-enable test_bbox_size with corrected limits in MPL 2 + 3 - #11010
Conversation
|
Not sure why CircleCI is issuing kill signal... I will have to defer to |
|
The jobs seem to fail early on in the setup/build stage; I've seen similar errors on failing to install astropy_helpers or importing from a source checkout when trying to build a fresh installation for Python 3.7 or 3.9. I might just have been lucky to have still had a partial 3.8 installation around. |
|
I think @astrofrog was going to fix the CI in 4.0.x. The CI there is currently broken. |
|
The CI is fixed, so this will need rebasing |
5fa3d94 to
270acfb
Compare
|
Hello @dhomeier! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-11-30 23:28:37 UTC |
Eh... I thought we set it to 100? |
|
CircleCI still |
Apparently not backported to 4.0.x. Still the astropy_helpers-related warnings and failures and Travis also seems to hang in the "Rebuilding" stage for several minutes now without any progress. |
|
Something isn't right. You have a commit that doesn't belong. Ah, right. I think @astrofrog force pushed to the branch. I think you might need to open a new PR and cherry pick your commits from here. |
|
I had just rebased 5 minutes ago, but then the unrelated commit still showed up here – before, I think a dozen extra commits had popped up, but maybe that was due to the force-push. |
|
I mean, Tom R force pushed to 4.0.x itself. That can mess up a PR opened based on old 4.0.x history that no longer exists. You can reset HEAD and stuff but I think at this point, it might be easier if you pull the new 4.0.x branch and open a new PR off it. |
270acfb to
9d55463
Compare
|
At least getting rid of a880b60 allows me to build locally again (maybe I just missed that from Tom's force-push?). |
|
At least no kill signal to CircleCI but now this: |
Which is existing here and apparently on Travis as well; CircleCI still seems to be rebasing against something else... |
|
4 seemingly unrelated failures on Travis; at least one step forward. |
|
That is still weird though. I thought @astrofrog said the CI is fixed. |
Can't identify exactly what setup steps CI is doing, but when I am reproducing first Travis's I only have |
|
I can try and look into the failures tomorrow, it possible that I didn't fix them all before. |
|
Oh, I think I might have spotted it. circleCI on the LTS branch still runs |
|
Let me know if you think I can help - am happy to pull a branch or so and test locally. |
|
Thank you @mhvk! I'm testing adding the install step in #11081, if that's not working then I would very much appreciate the help as I cannot yet reproduce the issue locally. |
|
🎉 - issue is solved, with some git magic @dhomeier suggested above. I don't fully see why it does the trick, but given it only affects the LTS, the "works for now"™️ is good enough for me. However, there are a few failures. Once it's all sorted out, I would suggest to rebase this PR as I made a bunch of small fixes to some of the backports and that messed up your base branch. |
9d55463 to
d94dd82
Compare
|
Can't make much sense of the first two jobs – both seem to point to the same run at @bsipocz – manually triggered a rerun some days later, that seems to have done it, but no idea about the original issue. |
pllim
left a comment
There was a problem hiding this comment.
Seems to have picked up an unrelated commit.
That popped up after rebasing onto v4.0.x. I could try to rebase again, other wise no idea how to get rid of it. |
|
Yeah, I guess you could try... 🤞 😬 🤞 |
|
Hasn't changed anything locally, HEAD is still directly on top of d69adc1... |
|
Looks like the branch was force-pushed again, so you need to do that thing you did to fix things after that other force-push. |
|
I've rebased about 5 times now, v4.0.x locally has its HEAD at ddf87c3, but the rebase branch still has d69adc1 on top of that. #10970 shows up as merged about 10 different times by @bsipocz, @taldcroft and myself (but seemingly all into this branch???) ; I'll be damned if I understand what is going on there! |
d94dd82 to
739de05
Compare
|
At least |
|
@dhomeier - GitHub keeps the references on all backports, even the ones that are amended. It was a bit of a back and forth... |
|
Must have picked up the branch at the wrong point in time then, I guess. At least the output from the failed win_x runs is now accessible; cache integrity problems seem unrelated. |
|
Stupid question: can I mark a change request as addressed other than dismissing it? |
Usually the reviewer will do that - so a gentle ping to @pllim. |
| assert np.allclose(ax_bbox.y1, 432) | ||
|
|
||
| # Enforce strict test only with reference Freetype version | ||
| if atol < 0.1 and not FREETYPE_261: |
There was a problem hiding this comment.
Instead of running it twice with different atol, why not run it once but set the atol according to FREETYPE_261?
There was a problem hiding this comment.
Yes, as noted in #11022 (comment) there are multiple redundant testing levels now that may not all be needed. Same perhaps for testing the other case as xfail rather than skipping it – I think yet other Freetype versions might fail on the stricter tolerance, but don't necessarily need to.
From a practical pov ~0.2 pt accuracy would probably be plenty for all purposes, but otoh for known mpl+ft versions we obviously can expect more precise results.
For consistency with #11022/v4.3 I'd probably leave it as is, but in the interest of keeping testing load low this can well do with a single run.
|
Thanks for the ping. I dismissed my change request but still have a question (see above). |
pllim
left a comment
There was a problem hiding this comment.
Being consistent is good, I agree. Merging this as-is then. Thanks!
|
Thanks for all the repair work on the branch! |
Description
LTS version of #10952 for accommodating
get_tightbboxcalculation changes over Matplotlib versions from #10900 (comment) (tested under MacOS X 10.14/Python 3.8 against mpl 2.2.5, 3.0.3 and 3.3.2).Since #11007 has turned up an additional case failing not on the upper boundaries
x1,y1but on (at least) one of the lower boundaries, not on different mpl versions but a different platform, we should wait until that issue is resolved.