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

add log safe Makie #116

Closed
wants to merge 6 commits into from
Closed

add log safe Makie #116

wants to merge 6 commits into from

Conversation

sfranchel
Copy link
Collaborator

@sfranchel sfranchel commented May 28, 2024

This is an attempt at making the MakieExt safe for plotting in log scale. But keeping the option to disable such a feature, and have histograms with negative values. Tagging @Moelf 😍

Copy link

codecov bot commented May 28, 2024

Codecov Report

Attention: Patch coverage is 44.44444% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 80.74%. Comparing base (e4bc8ca) to head (d45f246).
Report is 1 commits behind head on main.

Files Patch % Lines
ext/FHistMakieExt.jl 0.00% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #116      +/-   ##
==========================================
+ Coverage   80.37%   80.74%   +0.36%     
==========================================
  Files          11       12       +1     
  Lines         790      805      +15     
==========================================
+ Hits          635      650      +15     
  Misses        155      155              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Moelf
Copy link
Owner

Moelf commented May 28, 2024

🤩, thanks a bunch, will take a look after some meetings.

Comment on lines 9 to 16
const SAFE_LOG = Ref(true)

"""
set_safe_log(x::Bool)

Turn on/off SAFE_LOG, to plot histograms without/with negative values. By default is `true`.
"""
function set_safe_log(x::Bool)
Copy link
Owner

@Moelf Moelf May 28, 2024

Choose a reason for hiding this comment

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

I don't think we can add new things into the namespace via Pkg Extension mechanism.

julia> using Revise, CairoMakie, FHist
^LPrecompiling FHistMakieExt
  1 dependency successfully precompiled in 4 seconds. 222 already precompiled.

julia> FHist.SAFE_LOG
ERROR: UndefVarError: `SAFE_LOG` not defined
Stacktrace:
 [1] getproperty(x::Module, f::Symbol)
   @ Base ./Base.jl:31
 [2] top-level scope
   @ REPL[2]:1

julia> FHist.set_safe_log
ERROR: UndefVarError: `set_safe_log` not defined
Stacktrace:
 [1] getproperty(x::Module, f::Symbol)
   @ Base ./Base.jl:31
 [2] top-level scope
   @ REPL[3]:1

I will see if we can resolve this using plotting arguments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad, I just tested that the clipping was doing the right thing, but haven't check that the switch was actually working...

Copy link
Owner

Choose a reason for hiding this comment

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

yeah no worries, this is more than enough! I will try to make it nicer to change for users!

@sfranchel
Copy link
Collaborator Author

I did try to make this a bit more clean.

Now everything is in a submodule SafeLog. Default behaviour is to do not clip anything. Now we clip just for the plot, and not the actual histogram.

This is roughly how it should be working:

julia> h = Hist1D(bincounts=[0.5,-0.5],binedges=[1,2,3],sumw2=[1.01,1.01])
julia> f, ax, p = stairs(h)
julia> errorbars!(ax,h)

gives:
image

While setting log safe to true, clips to:

julia> FHist.SafeLog.set_safe_log(true)

image

And plotting in log scale does not throw errors! 🥳

@Moelf
Copy link
Owner

Moelf commented May 29, 2024

Personally not a fan of global options in packages if we can avoid them.

@sfranchel
Copy link
Collaborator Author

Personally not a fan of global options in packages if we can avoid them.

I can see that, I tried to put everything in a submodule to make "safer" and make the global value a reference. I'm not sure how to do this otherwise. But having such a feature would be very useful, but indeed it should be not the default behaviour.

el_vec[mask] = c_vec[mask] .- min_positive

# clip higher errors
@. eh_vec = max(eh_vec - (c_vec - c_vec_def), min_positive)
Copy link
Owner

Choose a reason for hiding this comment

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

what is the logic of this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The logic here is that if you camp bin counts, you need to adjust also the upper error accordingly. E.g. your data point is y = - 0.1 σ = 0.2. If you clip the central value to y' = 0, the upper error should be set to σ' = σ - (y' - y) = 0.1. Now if central value is very low or error is very small, you could get σ' < 0, and therefore you take the max between that and min_positive value. Does that make sense?

Copy link
Collaborator Author

@sfranchel sfranchel May 29, 2024

Choose a reason for hiding this comment

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

All this is in the spirit that even if you're clamping stuff to look at log scales, it could still be interesting information that there's a data point right below the 0, but with an error big enough that makes it compatible with a positive value, and up to which positive value the error extends. E. g. the second plot here.

@Moelf
Copy link
Owner

Moelf commented May 30, 2024

done in #118

@Moelf Moelf closed this May 30, 2024
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.

2 participants