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

added limited support for logarithmic units #72

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ykonter
Copy link

@ykonter ykonter commented May 1, 2022

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
Copy link

codecov-commenter commented May 1, 2022

Codecov Report

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

❗ 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%) ⬇️

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
Copy link
Collaborator

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
Copy link
Collaborator

gustaphe commented May 2, 2022

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
Copy link
Author

ykonter commented May 2, 2022

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
Copy link
Collaborator

gustaphe commented May 2, 2022

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
Copy link
Author

ykonter commented May 2, 2022

@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
Copy link
Collaborator

gustaphe commented May 3, 2022

That's cool.

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

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.

None yet

4 participants