-
-
Notifications
You must be signed in to change notification settings - Fork 344
add smart fillto to enable logscale y-axis stacked barplot #4849
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
base: master
Are you sure you want to change the base?
Conversation
Do I need to provide reference plots? is there anything else I should do in the meantime? |
A mention in the docstring about the source of the heuristic would be nice, but we (maintainers) have to update the reference images |
@asinghvi17 #3004 (comment) we discussed that here, I can't find the exact code now, ROOT does something depends on if minimum is >1 or <1, but approximately /2 in both cases. |
Bump |
Looks good to me! @jkrumbiegel, do you have any objections? |
I don't know, it's kind of too arbitrary for me with these "smart" fillto values, I also didn't like the other log one. But I also don't use these kinds of stacked barplots so I don't really mind either. |
I agree it's arbitrary but I think we also agree it shouldn't be empty, because then it's unusable. A nonarbitrary choice is to fill down to |
Since Axis relies on boundingbox() now and boundingbox() processes transformations you can have `-Inf`` in transformed data with sane limits by adding a method. If that's closer to the optimal solution I could update the pr for that. |
does that mean user must set limits themselves? or you're saying you can implement this |
The latter. You can implement the heurisitic for boundingbox so that Axis uses it for limits while using something like eps() for the actual data. |
in that case feel free to do that, and I very much appreciate it |
fix #4549
related to #3004 and refactored a bit of code
Before
After