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

Tweak Gain arithmetic #463

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

Tweak Gain arithmetic #463

wants to merge 3 commits into from

Conversation

Keno
Copy link
Contributor

@Keno Keno commented Jul 10, 2021

This tweaks gain arithmetic, by making both Gain and Level
either purely linear or purely logarithmic, semantically. Level
was already mostly linear, with a few minor exceptions that were
removed here. Gain was previously logarithmic for purposes of
+,- and linear for purposes of *,/ (i.e. both sets of
operations were equivalent - #461). This makes it purely logarithmic,
i.e. * and / between Gains are disallowed. When arithmetic
is mixed, only one of (+,-), (*,/) is disallowed. Which
one depends on whether the non-Gain unit involved is a Level
(in which case (+,-) is used) or a regular Quantity (in which
case (*,/) is used). Note that this design leaves all examples
that are currently in the docs on this topic unchanged (though
I did rewrite the docs to clarify the new semantics). I'll take
that as a hint that this is a reasonable design that is not too
breaking to people's expectations. I also think it's a lot more
consistent.

The primary advantage of doing this is that it'll allow us to
reclaim other arithmetic operations like *// on Gain,
while being quite consistent. For example, I'm workign with
an SDR that supports gains between 0.0dB and 1.0dB in its
amplifier, in setting increments of 1/256dB (linearlly spaced
in dB space, i.e. logarithmically spaced). I'd like to represent
this as 0.0dB:(1/256)dB:1.0dB, but with the current definition
of arithmetic, the definitions of range are unusable, because
the arithmetic system is iternally non-consistent.

This does not yet add those new operations, becuase some things
(like /(::Gain, ::Gain)) would change meaning, so I'd like to
give people a couple weeks to notice that things changed, but
I'm hoping to do that in the future.

cc @sostock - let me know what you think

Keno added 2 commits July 10, 2021 19:29
For gain the negation gives the negative gain, consistent with arithmetifc
operations operating in the logarithmic domain.

For level, negation is disallowed, consistent with treating level
as a linear quantity in most contexts.

Fixes PainterQubits#429
This tweaks gain arithmetic, by making both `Gain` and `Level`
either purely linear or purely logarithmic, semantically. `Level`
was already mostly linear, with a few minor exceptions that were
removed here. `Gain` was previously logarithmic for purposes of
`+`,`-` and linear for purposes of `*`,`/` (i.e. both sets of
operations were equivalent - PainterQubits#461). This makes it purely logarithmic,
i.e. `*` and `/` between Gains are disallowed. When arithmetic
is mixed, only one of `(+,-)`, `(*,/)` is disallowed. Which
one depends on whether the non-Gain unit involved is a Level
(in which case `(+,-)` is used) or a regular Quantity (in which
case `(*,/)` is used). Note that this design leaves all examples
that are currently in the docs on this topic unchanged (though
I did rewrite the docs to clarify the new semantics). I'll take
that as a hint that this is a reasonable design that is not too
breaking to people's expectations. I also think it's a lot more
consistent.

The primary advantage of doing this is that it'll allow us to
reclaim other arithmetic operations like `*`/`/` on `Gain`,
while being quite consistent. For example, I'm workign with
an SDR that supports gains between `0.0dB` and `1.0dB` in its
amplifier, in setting increments of `1/256dB` (linearlly spaced
in dB space, i.e. logarithmically spaced). I'd like to represent
this as `0.0dB:(1/256)dB:1.0dB`, but with the current definition
of arithmetic, the definitions of range are unusable, because
the arithmetic system is iternally non-consistent.

This does not yet add those new operations, becuase some things
(like `/(::Gain, ::Gain)`) would change meaning, so I'd like to
give people a couple weeks to notice that things changed, but
I'm hoping to do that in the future.
@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2021

Codecov Report

Merging #463 (de6cc5d) into master (1e78e6b) will decrease coverage by 0.29%.
The diff coverage is 75.00%.

❗ Current head de6cc5d differs from pull request most recent head 1d4f91b. Consider uploading reports for the commit 1d4f91b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #463      +/-   ##
==========================================
- Coverage   83.83%   83.53%   -0.30%     
==========================================
  Files          16       16              
  Lines        1342     1336       -6     
==========================================
- Hits         1125     1116       -9     
- Misses        217      220       +3     
Impacted Files Coverage Δ
src/user.jl 94.26% <ø> (ø)
src/utils.jl 95.34% <ø> (ø)
src/logarithm.jl 67.93% <75.00%> (-1.62%) ⬇️
src/quantities.jl 94.14% <0.00%> (-0.46%) ⬇️

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 2f4cfa2...1d4f91b. Read the comment docs.

@giordano giordano requested a review from sostock July 10, 2021 22:48
Copy link
Collaborator

@sostock sostock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This first review is only about the docs, I haven’t looked at the code yet.

@@ -134,11 +131,11 @@ logarithmic quantity is:
Unitful.Gain
```

One might expect that any gain / attenuation factor should be convertible to a pure number,
One might expect that any gain / attenuation factor should be convertible to a scalar,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that “pure number” is not a good term here, but I also don’t like “scalar”. To me, scalar means “not a vector”, and even though that is technically correct (dimensionful quantities form a vector space), I think it doesn’t make this more clear. I would write “real number”, but I see that you replaced that as well. Can you explain why you prefer “scalar” over “real number”?

If we agree on “scalar” here, it should also be used in the first sentence in the “Constructing logarithmic quantities” section.

docs/src/logarithm.md Outdated Show resolved Hide resolved
docs/src/logarithm.md Outdated Show resolved Hide resolved
docs/src/logarithm.md Outdated Show resolved Hide resolved
does not make much sense. This is because `dB` acts more like a constructor than a proper
unit.
With a few exceptions, dimensionful logarithmic units, such as `dBm` behave just like the underlying
linear unit for purposes of arithmetic (i.e. arithmetic operations commute with `linear`). However,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linear is not yet introduced at this point, so users will not know what this means.

docs/src/logarithm.md Outdated Show resolved Hide resolved
docs/src/logarithm.md Outdated Show resolved Hide resolved
docs/src/logarithm.md Outdated Show resolved Hide resolved
docs/src/logarithm.md Outdated Show resolved Hide resolved
docs/src/logarithm.md Outdated Show resolved Hide resolved
@sostock
Copy link
Collaborator

sostock commented Jul 12, 2021

We should definitely fix the arithmetic to be more consistent. However, I would consider this a breaking change, and if we change Gain arithmetic for a 2.0 release, I would prefer to fix other issues with the arithmetic as well, for example #402. Of course, this can happen in a separate PR, but maybe it is worth to work out a consistent arithmetic now and discuss what changes we want to make.

@sostock
Copy link
Collaborator

sostock commented Jul 12, 2021

Negating a Level throws an error, but subtracting a larger from a smaller level does not. I think this should be disallowed (it errors when printed):

julia> 2u"dBm" - 3u"dBm";

julia> show(ans)
ERROR: DomainError with -0.4103691225077659:
NaN result for non-NaN input.

@sostock sostock added the logarithmic logarithmic scales (decibels, nepers, …) label Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logarithmic logarithmic scales (decibels, nepers, …)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants