search

jw3126 / UnitfulRecipes.jl

Plots.jl recipes for Unitful.jl arrays
MIT License
37 stars 10 forks source link

limited support for logarithmic units including dB*m and dB/Hz #73

Open ykonter opened 2 years ago

ykonter commented 2 years ago

I hope you don't me starting a new pull request on logarithmic units. I decided on a new approach to support logarithmic units. This requires minimal changes to the existing code, but adds support for the four identified issues:

  1. What happens if you plot dBV and then plot! dBμV? -> works
  2. If you plot dBV, and then plot! V? -> works
  3. If you plot V and then plot! dBV? -> works
  4. Is dB/m treated well? -> dB/m, dB/Hz, dB*m are working

Note: I found that only the unit function in Unitful.jl required a fix. See the comments in the UnitfulWrapper.jl file. unit produces inconsistent results for logarithmic units. Fixing this within Unitful.jl caused a lot of things to break (a lot of math in Unitful requires unit to behave the way it does). As a temporary fix, I took the liberty of defining a new function fullunit which returns an appropriate unit instead.

Finally, Unitful.jl does not have a method ustrip(unit, quantity) for logarithmic units. I added one in the UnitfulWrapper.jl.

These changes are sufficient to support logarithmic units.

I will be very grateful if you'd consider merging this PR. Let me know if you have any thoughts or comments on the solution. Improvements are also welcome.

codecov-commenter commented 2 years ago

Codecov Report

Merging #73 (da05310) into master (759bf85) will increase coverage by 1.19%. The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
+ Coverage   86.11%   87.30%   +1.19%     
==========================================
  Files           1        2       +1     
  Lines         108      126      +18     
==========================================
+ Hits           93      110      +17     
- Misses         15       16       +1
Impacted Files Coverage Δ
src/UnitfulRecipes.jl 86.48% <55.00%> (+0.37%) :arrow_up:
src/UnitfulWrapper.jl 93.33% <93.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7741be7...da05310. Read the comment docs.

ykonter commented 2 years ago

Ok, I've added some test for the fullunit function. This should improve the code coverage to an acceptable level.