limited support for logarithmic units including dB*m and dB/Hz#73
Open
ykonter wants to merge 4 commits intojw3126:masterfrom
Open
limited support for logarithmic units including dB*m and dB/Hz#73ykonter wants to merge 4 commits intojw3126:masterfrom
ykonter wants to merge 4 commits intojw3126:masterfrom
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Author
|
Ok, I've added some test for the |
jw3126
reviewed
May 11, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
dBVand thenplot!dBμV? -> worksplotdBV, and thenplot!V? -> worksVand thenplot!dBV? -> worksdB/mtreated well? -> dB/m, dB/Hz, dB*m are workingNote: I found that only the
unitfunction in Unitful.jl required a fix. See the comments in the UnitfulWrapper.jl file.unitproduces 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 functionfullunitwhich 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.