Repository navigation
Fix Sphinx build on Travis - #5997
Conversation
|
Confirmed fixed -- https://travis-ci.org/astropy/astropy/jobs/227969317 Warnings are gone too but I don't know where to check the resultant plots in the doc for whether the tick labels actually look okay or not. @astrofrog ? |
|
Increasing timeout and the remote data tests passed but is this coincidence? 🤔 |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Nice find about the sphinx errors. My only worry is whether the wcsaxis solution is OK. If we don't hear from @astrofrog soon, I'd suggest making that a separate PR and merging the other commits.
| dist = np.hypot(dx, dy) | ||
|
|
||
| # Avoid RuntimeWarning | ||
| if dist == 0 or not np.isfinite(dist): |
There was a problem hiding this comment.
It seems OK, but really will have to ask @astrofrog whether this is reasonable. An alternative would be to just change the particular example to not hit this problem.
There was a problem hiding this comment.
I couldn't tell from the Travis log where the RuntimeWarning came from. I built the doc locally and did not see any difference in the generated images in the "Example gallery". Maybe I am looking in the wrong place? But regardless we need to address this one way or another, since ddx and ddy will not be a real number anyway. I am just not sure whether ddx = dx (in this PR) or ddx = 0 is more logical.
There was a problem hiding this comment.
Just let me know when you are done waiting for reply and want me to take this out.
Also, should we increase remote_timeout here or in a separate PR? Thinking about it more, that one is more like an API change and probably should have a change log.
There was a problem hiding this comment.
Can you make a regression test for this warning so that we can investigate in more detail what the best fix is?
|
This looks good to me, but I suggest to move the wcsaxes stuff into a separate PR now as that part is less critical and can wait for @astrofrog to catch up with things. |
|
On a second look, I think the solution for |
It is configurable. This is just changing the default. |
|
OK, is one option then to just configure it on travis only? |
That is an option but it would have to be done on a test by test basis. |
|
Also, personally I think 3 seconds is too short. Where did that number come from anyway? |
|
So this is what I'll do. I'll move the commits of remote timeout and wcsaxes into 2 other PRs and only keep Sphinx fix here. PRs incoming soon. |
|
Thanks. I've changed the milestone and will backport if this is a clean one. |
Fix #5992 . Also attempts to get rid of
RuntimeWarningas requested in #5992 (comment) .Was going to add something for #5994 here but perhaps that is no longer necessary since remote data server seems to be responsive again.