-
Notifications
You must be signed in to change notification settings - Fork 112
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
Implement logarithmic quantities #103
Conversation
Changes Unknown when pulling 36aa3a5 on logunits into ** on master**. |
Wow there's an impressive amount of work that's gone into this. It looks really nice! However, I find this decision really confusing:
I think there is an argument to be made that For example, my
For similar reasons, I also think Additional food for thought. In astronomy, the brightness of a star is measured in terms of "magnitudes", which are computed as |
Some responses:
|
I am in support of I think the current design has two contradicting philosophies. The first is that a linear and logarithmic units are interchangeable. This means that if I write a function that operates on a power quantity, I shouldn't have to care if that power is given to me in The second is that logarithmic units should operate in the way you expect. If I have a two The first implies that the quantity Either we have to live with a surprising behavior that makes it difficult to write generic functions that accept both logarithmic and linear units, or we have to accept that gains are applied by multiplication in all cases (and forbid As for the root-power quantities vs power quantities, maybe it makes sense to force the user to state which one they want when they write down |
Fortunately it's not a large design change to make It is worth keeping in mind that Regarding your last point, neither is more common, or at least whatever decision I make would be disappointing to a fair number of people. You can measure scattering parameters with a network analyzer and get magnitudes in dB corresponding to voltage ratios, which I do pretty regularly. Of course signal power attenuation (power ratios) are also pretty important. I think it is important to avoid silently returning the wrong answer when one expects something to work in their context, since that is also surprising behavior. |
… multiplication tables in docs based on latest behavior.
src/logarithm.jl
Outdated
irp = isrootpower(x) | ||
str = ifelse(irp, "root-power", "power") | ||
warn("result may be incorrect. Define ", | ||
"`Unitful.isrootpower(::Type{<:Unitful.LogInfo}, ::typeof($y))` to fix.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little wary of encouraging the user to change the behavior of Unitful because it makes the package behavior unpredictable. If I write some code using Unitful and copy it over to somebody else's system, I wouldn't want it to do a different thing just because they defined isrootpower
differently in their .juliarc.jl
for some units.
Maybe keep the behavior of dB
as is, but in ambiguous cases where it's not clear whether you have a power or root-power quantity, then you can warn that the results may be incorrect, but encourage people to use either dB_power
or dB_amplitude
for which isrootpower
is always false
and true
respectively.
That way if I'm operating on voltages or powers, I can do x*10dB
because the convention is clear, but if I'm operating on some other quantity then I have to specify x*20dB_power -> 100x
or x*20dB_amplitude -> 10x
.
Just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see where you're coming from but I'm not sure the danger is high in this case. I guess you need to appeal to some physical laws to decide whether to use root-power or power for a quantity of given dimension. I can't think of any examples where the decision would be ambiguous. In other words, the behavior will not depend on user preferences: I would expect all users should define isrootpower
the same way for a given dimension, and if they don't, they would be responsible for the error (or receive a warning if they don't define anything at all).
Anyway, rather than defining dB_power
or dB_amplitude
, I guess I'd prefer an alternative, such preferring to return no results rather than possibly erroneous ones, and encouraging a pull request rather than local modifications. It'd be nice to "crowd source" the correct answers for each dimension in the absence of some general algorithm.
Codecov Report
@@ Coverage Diff @@
## master #103 +/- ##
========================================
Coverage ? 84.3%
========================================
Files ? 15
Lines ? 803
Branches ? 0
========================================
Hits ? 677
Misses ? 126
Partials ? 0
Continue to review full report at Codecov.
|
Changes Unknown when pulling c31e249 on logunits into ** on master**. |
I briefly experimented with the new user log units.
(where Jy is defined here) However, I immediately ran into
Does this mean
|
Yes, that's right, I'll add something like that in the next commit. |
Changes Unknown when pulling f4c73d6 on logunits into ** on master**. |
Ok, I'm somewhat happy with the current state of things. I added some functionality to allow for dimensionless reference levels, which enables dBFS (reference level is 1). Also, The remaining obviously wonky thing is that we have both I updated the docs for this branch again, should be easier to read than looking through the markdown files: http://ajkeller34.github.io/Unitful.jl/logunits/ Any other concerns? Maybe we should keep this open a little longer while you see if you encounter issues when playing with magnitude scales? |
Changes Unknown when pulling 64e1f03 on logunits into ** on master**. |
Apparently this:
I'll give it a shot when I find some more free time. You could also just merge and stick a big experimental warning on it. That'll get more people experimenting with log units, which will hopefully make it clear where the pain points are in the current design. |
I've been struggling with this all day and finally got it to work. I had to do a lot of digging and I think the documentation could benefit from some clarification. I deal with logarithmic quantities all the time in astronomy so this is quite useful to me. julia> using Unitful
julia> import Unitful:d
julia> @logscale Log₁₀ "Log₁₀" LogBase10 10 1 false
Log₁₀
julia> @logunit log₁₀d "log₁₀(days)" LogBase10 1d
log₁₀(days)
julia> Unitful.isrootpower_dim(::typeof(dimension(d))) = false
julia> foo = 3*log₁₀d
3.0 log₁₀(days)
julia> uconvert(u"d", foo)
1000.0 d
julia> bar = 0.3*log₁₀d
0.3 log₁₀(days)
julia> uconvert(u"d", bar)
1.9952623149688795 d The julia> foo = 3*log₁₀d
ERROR: undefined behavior. Please file an issue with the code needed to reproduce.
Stacktrace:
[1] error(::String) at ./error.jl:33
[2] isrootpower_dim(::Unitful.Dimensions{(Unitful.Dimension{:Time}(1//1),)}) at /home/mark/.julia/packages/Unitful/W0mMi/src/logarithm.jl:200
[3] isrootpower(::Quantity{Int64,𝐓,Unitful.FreeUnits{(d,),𝐓,nothing}}) at /home/mark/.julia/packages/Unitful/W0mMi/src/logarithm.jl:199
[4] fromlog(::Type, ::Quantity{Int64,𝐓,Unitful.FreeUnits{(d,),𝐓,nothing}}, ::Int64) at /home/mark/.julia/packages/Unitful/W0mMi/src/logarithm.jl:116
[5] *(::Int64, ::Unitful.MixedUnits{Level{Unitful.LogInfo{:LogBase10,10,1},1 d,T} where T<:Number,Unitful.FreeUnits{(),NoDims,nothing}}) at /home/mark/.julia/packages/Unitful/W0mMi/src/logarithm.jl:147
[6] top-level scope at none:0 |
Side comment just to say that you may be interested in https://github.com/JuliaAstro/UnitfulAstro.jl, if you didn't already see it |
@giordano I do indeed use UnitfulAstro |
Here's my first pass at implementing logarithmic quantities. It seems to mostly work, but I would appreciate hearing about things that don't work but should, things that do work but shouldn't, and any objections to design decisions. In a perfect world I would keep this open for a week or so, then merge and tag, but that will depend on how much feedback I get.
Please see
docs/src/logarithm.md
for a full description.