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

Fix: adapted grid to work with Number #110

Merged
merged 2 commits into from
Jan 6, 2021
Merged

Conversation

briochemc
Copy link
Contributor

This PR simply extends the type of the (min, max)-tuple argument for the adaptive_grid to accept Numbers instead of just Reals. The goal is to fix jw3126/UnitfulRecipes.jl#36 🙂.

For example, with this PR, this MWE

using Unitful: m
using Plots, UnitfulRecipes
f(x) = sin(x / m)
plot(f, -5m, 5m)

would give
Screen Shot 2021-01-06 at 10 42 03 am
which otherwise currently throws a

MethodError: no method matching adapted_grid(..., ::Tuple{Quantity, Quantity})

on master.

Hopefully, this PR does not break anything since it's a tiny edit! (I have not run tests locally so waiting to see the CI runs... 😁)

@briochemc
Copy link
Contributor Author

Well... Every CI workflow failed, so I guess this is not as simple a fix I thought! 😅

@briochemc
Copy link
Contributor Author

I did not realize that the types of min and max could be different, which is why the CI failed before, so I just fixed that in the latest commit. Hopefully this works! 🤞

@briochemc
Copy link
Contributor Author

The CI fails seem on these zscale tests:

if VERSION v"1.5"
@test cmin 2827.440 atol = 1e-3
@test cmax 4172.194 atol = 1e-3
else
@test cmin 2823.715 atol = 1e-3
@test cmax 4185.414 atol = 1e-3
end

which seems unrelated to this PR. BTW, since these seem to be Julia-version dependent (no idea why), they might need to be updated for Julia v1.6, v1.7, and so on?

@briochemc
Copy link
Contributor Author

Just in case, cc @mileslucas for the zscale CI failures? 🤷

@mileslucas
Copy link
Contributor

mileslucas commented Jan 6, 2021

test failures for zscale are due to ever-changing random number generation 😞. I'll open a separate issue to triage this- I don't think it should block this PR.

@mileslucas mileslucas mentioned this pull request Jan 6, 2021
@briochemc
Copy link
Contributor Author

@daschw maybe this can be reviewed for merging before the next release? (Saying this because I saw there was PR #109.)

@daschw
Copy link
Member

daschw commented Jan 6, 2021

Nice! Thanks a lot!

@daschw daschw merged commit d839aa0 into JuliaPlots:master Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recipe for parametric with units?
3 participants