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

ENH: Add dBZ as a unit in the registry #3343

Merged
merged 6 commits into from
Jan 11, 2024

Conversation

mgrover1
Copy link
Contributor

@mgrover1 mgrover1 commented Jan 2, 2024

Description Of Changes

Fixes #3341

Adds "dBZ" as a unit within the unit registry, with this being equivalent to dB, while still being its own separate unit.

Checklist

@mgrover1 mgrover1 requested a review from a team as a code owner January 2, 2024 16:46
@mgrover1 mgrover1 requested review from dopplershift and removed request for a team January 2, 2024 16:46
@dcamron
Copy link
Member

dcamron commented Jan 2, 2024

Thanks for putting this together! With some hand testing, this seems to cooperate with cross_section and quantification/dequantification. Given Pint's "beta" qualifier for log units (and their inherent quirks), I'm a little worried about how this could potentially interact with some of our other unit handling in calculations, eg

>>> from metpy.units import units
>>> ref = 40 * units.dBZ
>>> ref
<Quantity(40, 'dBZ')>
>>> ref.magnitude
40
>>> ref.to_base_units()
<Quantity(10000.0, 'dimensionless')>
>>> ref.m_as('')
10000.00000000001

At first pass, I can't find anywhere we do this kind of handling that dBZ could even sneak into, but might be something to be mindful of or test for in the future. I'll leave that thought out there for other inputs.


Actual review questions: Should there be a descriptive longname, decibel_reflectivity or something like that? And is dBz a helpful alias here as well? Not sure what're valid names and units in this space, personally.

@mgrover1
Copy link
Contributor Author

mgrover1 commented Jan 2, 2024

@dcamron thanks for the thoughts here;

dBZ is a dimensionless quantity so that behavior is expected, and according to the cf-conventions, dBZ is the proper units for this quantity.
https://cfconventions.org/Data/cf-standard-names/28/build/cf-standard-name-table.html

equivalent_reflectivity would be a suitable name for the variable name, but these are the units. The units are in decibels of reflectivity, or dBZ.

It may be helpful to add BZ here as well, since that is mentioned as the "base unit" in this PR @dopplershift submitted a few years ago

https://github.com/Unidata/UDUNITS-2/pull/42/files

Any other thoughts here?

@dopplershift
Copy link
Member

dopplershift commented Jan 3, 2024

I'm not sure I agree that dBz is necessarily "dimensionless"--it's a log quantity relative to 1 mm^6/m^3. dB is dimensionless because it's the 10 log10 of some number that is itself dimensionless. But if you convert 40 dBz to linear units, you should get 10000 mm^6/m^3. I don't recall, does Pint's beta log support allow us to specify that?

Same thing would be true of dBm, which is dB relative to 1 mW.

My first question, though: why is this suddenly an issue with MetPy 1.6?

@dopplershift
Copy link
Member

To answer my question above, it's because I added a dequantify()/quantify() pair in interpolate_to_slice() when cleaning up some unit warnings and now it needs to actually parse the units. 😞

@dopplershift
Copy link
Member

Ok, I think we can get what we want here with:

units.define('dBz = 1e-18 m^3; logbase: 10; logfactor: 10 = dBZ')

allows us to do:

ref = 55 * units.dBz
ref.to('mm^6/m^3')
ref.to_base_units()

and get 316227.7660168382 millimeter ** 6/meter ** 3 and 3.162277660168382e-13 meter ** 3, which are dimensionally correct.

Strong preference for dBz as the main name for the unit on the basis of correctness, but I recognize we need to have dBZ as an alias just for practicality.

Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

I think with a few tweaks this will be a nice thing to include.

src/metpy/units.py Outdated Show resolved Hide resolved
tests/units/test_units.py Outdated Show resolved Hide resolved
@dopplershift
Copy link
Member

I should add that I tried to define the linear equivalent directly as a mm^6/m^3, but I kept getting

DimensionalityError: Cannot convert from '1 / m ** 3' (1 / [length] ** 3) to 'millimeter ** 6 / meter ** 3' ([length] ** 3)

so I assume there's an issue with the more complicated units and the Pint parser. It's fine, we're still correct, the only downside is that to_base_units() takes us to m^3, which ends up as really tiny numbers.

This does show there's still work to do on the Pint side, since:

50 * units.dBZ + 10 * units.dB

gives DimensionalityError: Cannot convert from 'dBz' ([length] ** 3) to 'decibel' (dimensionless) even though it should work just fine.

@dopplershift dopplershift added Type: Enhancement Enhancement to existing functionality Area: Units Pertains to unit information labels Jan 3, 2024
@dopplershift dopplershift added this to the January 2024 milestone Jan 3, 2024
Co-authored-by: Ryan May <rmay31@gmail.com>
@mgrover1
Copy link
Contributor Author

mgrover1 commented Jan 3, 2024

Thanks for the feedback here @dopplershift !! I added in those suggestions. Please let me know if you have any other comments/changes you would like to see :)

@dcamron
Copy link
Member

dcamron commented Jan 3, 2024

I should add that I tried to define the linear equivalent directly as a mm^6/m^3, but I kept getting DimensionalityError

@dopplershift I had actually tried this yesterday as well before I commented, and it works if you first define Z and then define dBz as a log-unit of Z,

reg.define('reflectivity_factor = mm^6 / m^3 = _ = Z')
reg.define('dBZ = Z ; logbase: 10; logfactor: 10 = dBz')

I had just abandoned the thought until the conversation headed that way anyway.

@mgrover1
Copy link
Contributor Author

mgrover1 commented Jan 3, 2024

@dcamron @dopplershift should I add this then?

I should add that I tried to define the linear equivalent directly as a mm^6/m^3, but I kept getting DimensionalityError

@dopplershift I had actually tried this yesterday as well before I commented, and it works if you first define Z and then define dBz as a log-unit of Z,

reg.define('reflectivity_factor = mm^6 / m^3 = _ = Z')
reg.define('dBZ = Z ; logbase: 10; logfactor: 10 = dBz')

I had just abandoned the thought until the conversation headed that way anyway.

@dopplershift
Copy link
Member

@mgrover1 Don't worry about it. I'm really iffy on Z being listed as a formal unit, and I don't think the use cases justify having it.

While this is definitely a good thing to add, the plan also is to push this off until 1.7.0. In the meanwhile, we're going to put out a 1.6.1 that more directly addresses the problem with interpolate_to_slice().

@mgrover1
Copy link
Contributor Author

mgrover1 commented Jan 3, 2024

@mgrover1 Don't worry about it. I'm really iffy on Z being listed as a formal unit, and I don't think the use cases justify having it.

While this is definitely a good thing to add, the plan also is to push this off until 1.7.0. In the meanwhile, we're going to put out a 1.6.1 that more directly addresses the problem with interpolate_to_slice().

This sounds great! Thank you so much!

Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Looks good!

@dopplershift
Copy link
Member

With 1.6.1 out the door, we can go ahead and put this in main and start the crawl towards 1.7.

@dopplershift dopplershift merged commit 45730bb into Unidata:main Jan 11, 2024
34 checks passed
@dopplershift dopplershift removed this from the 1.7.0 milestone Jun 17, 2024
@dopplershift dopplershift added this to the 1.6.2 milestone Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Units Pertains to unit information Type: Enhancement Enhancement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UndefinedUnitError: 'dBZ' is not defined in the unit registry
3 participants