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

ecdf plot plus internal fixes #1444

Closed
wants to merge 11 commits into from
Closed

ecdf plot plus internal fixes #1444

wants to merge 11 commits into from

Conversation

piever
Copy link
Contributor

@piever piever commented Nov 12, 2021

@sethaxen, I'm continuing #1310 here, because it looks like it uncovered a small issue in Makie's conversion mechanism that should be addressed at the same time.

@@ -150,7 +150,7 @@ function apply_convert!(P, attributes::Attributes, x::PlotSpec{S}) where S
for (k, v) in pairs(kwargs)
attributes[k] = v
end
return (plottype(P, S), args)
return (S, args)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimonDanisch, I'm trying your suggestion here (which fixes the ECDFPlot bug) but I was wondering: should this actually be (plottype(S, P), args), so that the plot type suggested by the conversion pipeline, i.e. S, takes precedence, but if that is Any, it falls back to P?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... maybe that was the intention... Yeah, if plottype(S, P) does that, it sounds like a plan ;)

@sethaxen
Copy link
Contributor

sethaxen commented Nov 12, 2021

@sethaxen, I'm continuing #1310 here, because it looks like it uncovered a small issue in Makie's conversion mechanism that should be addressed at the same time.

@piever it would be cleaner to separate the ECDF plot PR, which does not touch Makie's internals, from this PR, which does change the internals of Makie. I'd prefer if you made the necessary changes separately and then I use them in that PR.

@piever
Copy link
Contributor Author

piever commented Nov 12, 2021

@piever it would be cleaner to separate the ECDF plot PR, which does not touch Makie's internals, from this PR, which does change the internals of Makie. I'd prefer if you made the necessary changes separately and then I use them in that PR.

Sure! Closing this in favor of #1310 and #1445 then

@piever piever closed this Nov 12, 2021
@sethaxen
Copy link
Contributor

Thanks!

@piever piever deleted the pv/ecdf branch January 14, 2022 09:53
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.

3 participants