-
Notifications
You must be signed in to change notification settings - Fork 10
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
Support units in clim (colorbar limits) #59
Conversation
Codecov Report
@@ Coverage Diff @@
## master #59 +/- ##
==========================================
+ Coverage 85.71% 86.66% +0.95%
==========================================
Files 1 1
Lines 91 105 +14
==========================================
+ Hits 78 91 +13
- Misses 13 14 +1
Continue to review full report at Codecov.
|
@jw3126 should "Moar" be "More" runtests.jl ? I could fix that too here but unsure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! I think this is the right approach. Always feel free to correct any spelling errors, like the Moar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@JeffFessler if you need an immediate release, you can bump the version. |
oops, thought i did that already! done - thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are just suggestions but I think they are worth trying?
src/UnitfulRecipes.jl
Outdated
# Recipe for contour|heatmap|surface(z) | ||
# Caution: also gets use for plot(z::Matrix) | ||
@recipe function f(z::AMat{T}) where T <: Quantity | ||
u = get(plotattributes, :zunit, unit(eltype(z))) | ||
clims_types = (:contour, :contourf, :heatmap, :surface) | ||
if get(plotattributes, :seriestype, :nothing) ∈ clims_types | ||
ustripattribute!(plotattributes, :clims, u) | ||
z = fixaxis!(plotattributes, z, :z) | ||
append_unit_if_needed!(plotattributes, :colorbar_title, u) | ||
else # plot(z::Matrix) | ||
z = fixaxis!(plotattributes, z, :y) | ||
append_unit_if_needed!(plotattributes, :y, u) | ||
end | ||
z | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is partially redundant with the recipe lines 11–14. Could you try and remove these lines here (59–73) and replace lines 11–14 with something like:
@recipe function f(::Type{T}, x::T) where T <: AbstractArray{<:Union{Missing,<:Quantity}}
axisletter = plotattributes[:letter] # x, y, or z
if (axisletter == :z) && (get(plotattributes, :seriestype, :nothing) ∈ (:contour, :contourf, :heatmap, :surface))
u = get(plotattributes, :zunit, unit(eltype(x)))
ustripattribute!(plotattributes, :clims, u)
append_unit_if_needed!(plotattributes, :colorbar_title, u)
end
fixaxis!(plotattributes, x, axisletter)
end
and see if this works the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW I feel like this could actually be put in the fixmarker
function (and maybe rename it)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You were correct that putting it in lines 11-14 was sufficient so I've done it that way now and cut the redundant code.
I didn't understand the suggestion about fixmarker
, and I don't really understand recipes but I like to try to make PR when I report an issue just to try to be help in part. I made this PR "editable by maintainers" so feel free to modify if you think there is an even simpler way.
.gitignore
Outdated
@@ -5,6 +5,7 @@ | |||
.DS_Store | |||
Manifest.toml | |||
dev/ | |||
.*.*.swp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does *swp
work here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just learned that I should be using a global configuration instead, so I will revert that change.
https://stackoverflow.com/questions/4824188/git-ignore-vim-temporary-files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I think it's fine to add this stuff to the .gitignore, I just thought *swp
would catch it all and be simpler.
Make heatmaps its own subsection in the examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just changed moved the heatmap example into its own little section, but otherwise looks good to me! Thanks for all the work!
LGTM. Thanks for the help. |
This addresses #57 and #58.
Please check carefully my work on #58 because it was tricky to me.
It seems that
plot(rand(5,4))
andheatmap(rand(5,4))
both have Matrix arguments. So I suspect that we need may need separate recipes for plot and for heatmap/surface/others? but I don't know enough about recipes to be confident in my solution.