search

jw3126 / UnitfulRecipes.jl

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

added limited support for logarithmic units #72

Open ykonter opened 2 years ago

ykonter commented 2 years ago

Dear all,

I'd like to contribute to the issue on support for logarithmic units. I've made some modifications and my first simple tests seem promising. Please let me know your thoughts on these modifications.

Kind regards,

Yuri

codecov-commenter commented 2 years ago

Codecov Report

Merging #72 (b3e5ac3) into master (759bf85) will decrease coverage by 2.91%. The diff coverage is 59.37%.

:exclamation: Current head b3e5ac3 differs from pull request most recent head 0cfff5c. Consider uploading reports for the commit 0cfff5c to get more accurate results

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
- Coverage   86.11%   83.20%   -2.92%     
==========================================
  Files           1        1              
  Lines         108      125      +17     
==========================================
+ Hits           93      104      +11     
- Misses         15       21       +6
Impacted Files Coverage Δ
src/UnitfulRecipes.jl 83.20% <59.37%> (-2.92%) :arrow_down:

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...0cfff5c. Read the comment docs.

briochemc commented 2 years ago

I don't have much time to look at this myself, but could you add one (or more) examples of what this PR adds?

gustaphe commented 2 years ago

Nice!

Just checking (I'll be able to test this out myself later, just putting it in writing now so I don't forget):

  1. What happens if you plot dBV and then plot! dBμV?
  2. If you plot dBV, and then plot! V?
  3. If you plot V and then plot! dBV?
  4. Is dB/m treated well?
ykonter commented 2 years ago

Thanks a lot for your considerations!

@gustaphe:

  1. What happens if you plot dBV and then plot! dBμV? -> I have overlooked this functionality ... these currently do not work as desired... I've added some 'uconvert' in my local version of the code which seems to solve this issue. I will do some more testing before submitting a revision.

  2. If you plot dBV, and then plot! V? This requires an additional conversion somewhere... am looking into it...

  3. If you plot V and then plot! dBV? This works with the modifications in 1.

  4. Is dB/m treated well? -> Unfortunately not. dB/m is treated as a "Quantity", but uconvert does not work for this. This is also the case for dB/Hz and similar "mixed" variations of logarithmic & linear units. I will need some more time to think about how to deal with these units.

Please let me know if there is any other functionality that I'm overlooking.. It would be great to have all features working for logarithmic units :)

gustaphe commented 2 years ago

I'm not saying, by the way, that this one PR necessarily solves 4. It's nice if it can, but incremental progress is also progress. 1 -- 3 are more important since a user might prefer an error to silently getting an incorrect scaling.

It might be worth investigating getting Unitful to fix unit and uconvert(u, q) for logarithmic units. But that doesn't exclude merging this temporarily.

ykonter commented 2 years ago

@gustaphe I agree with you on all points. I think the best solution is to fix/improve the ustrip and unit functions in Unitful. However, the Unitful code is a bit much for me to solve in my spare time. I found it easier to fix this here; albeit for the time being. With that in mind, I modified the solution by adding two functions _unit and _ustrip. These functions 'fix' the originals from Unitful. Once Unitful is up to speed, we can replace these with their originals unit and ustrip.

My latest commit should solve points 1 -- 3. It, however, does not include sufficient test scripts. I will add some in my next commit. Please wait with this PR until I've added some more tests.

Point 4 is also very high on my "want to have" list.. but will require some more effort.

gustaphe commented 2 years ago

That's cool.

I took the liberty of creating https://github.com/PainterQubits/Unitful.jl/pull/535 yesterday, addressing ustrip. unit seems like it works the way it's intended, though I'm not sure what the motivation is.