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

Add some common units #154

Merged
merged 4 commits into from
Nov 7, 2024
Merged

Add some common units #154

merged 4 commits into from
Nov 7, 2024

Conversation

Ickaser
Copy link
Contributor

@Ickaser Ickaser commented Nov 1, 2024

In the spirit of #153 and #152 , adding some more units with prefixes, since I ran into the same situation as #151 of wanting to register new units.

Specifically, adding atmosphere (atm), Torr, and cal as pressure and energy units respectively. The fact that people in my field commonly use these somewhat-annoying units is one of the main reasons I use packages like this and Unitful.

I would be willing to go through e.g. https://github.com/PainterQubits/Unitful.jl/blob/master/src/pkgdefaults.jl and add more of the default units listed there, to avoid needing a multitude of this kind of pull request.

@SymbolicML SymbolicML deleted a comment from sweep-ai bot Nov 1, 2024
src/units.jl Outdated Show resolved Hide resolved
src/units.jl Outdated Show resolved Hide resolved
@MilesCranmer
Copy link
Member

I would be willing to go through e.g. https://github.com/PainterQubits/Unitful.jl/blob/master/src/pkgdefaults.jl and add more of the default units listed there, to avoid needing a multitude of this kind of pull request.

Basically the way I am adding units is only if people have a specific need for them. So please only request units and prefixes that you use in your own work. This is to prevent bloating the library with units that are never used. Hopefully that sounds reasonable.

Co-authored-by: Miles Cranmer <[email protected]>
Copy link
Contributor

github-actions bot commented Nov 4, 2024

Benchmark Results

main d4f4b50... main/d4f4b509e3160d...
Quantity/creation/Quantity(x) 3.1 ± 0.01 ns 4.94 ± 0.01 ns 0.629
Quantity/creation/Quantity(x, length=y) 3.73 ± 0.01 ns 3.73 ± 0.01 ns 1
Quantity/with_numbers/*real 3.1 ± 0.01 ns 4.02 ± 0.92 ns 0.773
Quantity/with_numbers/^int 8.37 ± 1.6 ns 8.37 ± 1.6 ns 1
Quantity/with_numbers/^int * real 8.68 ± 1.6 ns 8.68 ± 1.6 ns 1
Quantity/with_quantity/+y 4.35 ± 0 ns 4.35 ± 0.01 ns 1
Quantity/with_quantity//y 3.42 ± 0.011 ns 3.42 ± 0.01 ns 1
Quantity/with_self/dimension 2.79 ± 0 ns 2.79 ± 0 ns 1
Quantity/with_self/inv 3.11 ± 0.001 ns 3.11 ± 0.001 ns 1
Quantity/with_self/ustrip 2.79 ± 0.01 ns 2.79 ± 0.01 ns 1
QuantityArray/broadcasting/multi_array_of_quantities 0.0847 ± 0.00051 ms 0.0848 ± 0.00078 ms 0.999
QuantityArray/broadcasting/multi_normal_array 0.0498 ± 0.00028 ms 0.0497 ± 0.00025 ms 1
QuantityArray/broadcasting/multi_quantity_array 0.0531 ± 0.00026 ms 0.0623 ± 0.00031 ms 0.852
QuantityArray/broadcasting/x^2_array_of_quantities 16.4 ± 9.4 μs 21.3 ± 4.3 μs 0.77
QuantityArray/broadcasting/x^2_normal_array 2.24 ± 0.91 μs 2.82 ± 3.2 μs 0.797
QuantityArray/broadcasting/x^2_quantity_array 6.5 ± 0.08 μs 6.52 ± 0.2 μs 0.997
QuantityArray/broadcasting/x^4_array_of_quantities 0.0782 ± 0.00045 ms 0.0782 ± 0.00064 ms 1
QuantityArray/broadcasting/x^4_normal_array 0.0497 ± 0.00025 ms 0.0498 ± 0.00022 ms 1
QuantityArray/broadcasting/x^4_quantity_array 0.0499 ± 0.00023 ms 0.0499 ± 0.00016 ms 1
time_to_load 0.196 ± 0.0015 s 0.197 ± 0.0042 s 0.993

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@Ickaser
Copy link
Contributor Author

Ickaser commented Nov 4, 2024

I would be willing to go through e.g. https://github.com/PainterQubits/Unitful.jl/blob/master/src/pkgdefaults.jl and add more of the default units listed there, to avoid needing a multitude of this kind of pull request.

Basically the way I am adding units is only if people have a specific need for them. So please only request units and prefixes that you use in your own work. This is to prevent bloating the library with units that are never used. Hopefully that sounds reasonable.

I understand that philosophy, and the rationale for not allowing global unit registering as you explained at #151 . But it does become a barrier to switching away from e.g. Unitful or pint, where I have basically never needed to register new units because these kinds of units are already defined; speaking as a chemical engineering student, one of the main reasons I want to use a package like this is that there are still quite a few cases where fields still have not fully made the switch to SI units. If all I had to deal with were plain SI units, I would just write everything in SI marks and not worry about conversions. Verifying correct dimensionality comes as a nice sanity check, but it's not what attracts me to the package.

There is also some amount of friction, where if I often need to deal with a unit (e.g. cal) I have to ask at what level I should be registering the unit. In my startup.jl? That seems weird, and doesn't port well to other people. At the start of every script? It's annoying to have that added to my boilerplate, etc.

Strictly speaking, I only need Torr, mTorr and cal, so I can take atmospheres out of this pull request if you prefer. But in general, I think a units package is more useful if you don't need to define your own units so often.

@MilesCranmer
Copy link
Member

Oops I just remembered, atm is already in the package but is listed in the constants module:

@register_constant atm 101325 * U.Pa

@MilesCranmer
Copy link
Member

MilesCranmer commented Nov 4, 2024

Do you use cal or kcal? I would feel more comfortable adding kcal and avoiding cal entirely, due to the ambiguity.

@MilesCranmer
Copy link
Member

doesn't port well to other people

I think for cases like this, this is a feature, not a bug. We don’t want to have a definition of cal that is unexpected — it could cause calculations to be wrong but in subtle ways. Sometimes safety needs to be emphasised in the design over convenience

@MilesCranmer
Copy link
Member

Another way of saying this: it's always better to have something fail loudly

@Ickaser
Copy link
Contributor Author

Ickaser commented Nov 4, 2024

Do you use cal or kcal? I would feel more comfortable adding kcal and avoiding cal entirely, due to the ambiguity.

Like I said, we have a heat transfer coefficient given in cal/s/K/cm^2 in even recent papers, with a history of those units in the field. Using kcal would be less ambiguous, but wouldn't achieve my goal here, unfortunately.

The US usage of "nutritional calorie" as 4184 J is, I feel, restricted to nutrition, and even reading the entire Wikipedia article it seems clear to me that the authors of the article had bought into the general scientific definition of 4.184 J. It's the kind of thing that got brought up in my high school chemistry class, so to me it feels like, technically, a possible footgun but not one that I have ever seen as a real problem. (In part, that's because the food definition of a "calorie" is not exactly correlated to what a biological system can use, so what you read on food packaging isn't ever really convertible to other units).

All that said, if the risk of someone trying to do math with the values reported on a US food label is more than you are willing to accept, then I can try to find a way to reasonably register calories for my scripts (or just default back to Unitful).

@MilesCranmer
Copy link
Member

I think nutritional calories are for more commonly used than the gram calorie, as it's literally on every nutrition label in the US. It just seems like a massive footgun to me to say that this calorie, that every adult in the US intuitively understands, is different from the one used in this package. Even if other units packages do this, I think it's a mistake. I just don't see convenience for a subset of users as being a strong enough argument for introducing this in a package that targets a broad user base.

The @register_unit macro is built for this exact purpose though! Is there any fundamental reason you wish to avoid it? You can also just do

const cal = 4.184u"J"

I'm happy to add Torr though. Also note that atm is already in the package in the constants submodule - though perhaps it could be "aliased" to the units module somehow. But the current code in this PR will error since atm is already a symbol.

@MilesCranmer
Copy link
Member

While we are at it I suppose we should add mmHg as well.

@Ickaser
Copy link
Contributor Author

Ickaser commented Nov 5, 2024

I think nutritional calories are for more commonly used than the gram calorie, as it's literally on every nutrition label in the US. It just seems like a massive footgun to me to say that this calorie, that every adult in the US intuitively understands, is different from the one used in this package. Even if other units packages do this, I think it's a mistake. I just don't see convenience for a subset of users as being a strong enough argument for introducing this in a package that targets a broad user base.

The @register_unit macro is built for this exact purpose though! Is there any fundamental reason you wish to avoid it? You can also just do

const cal = 4.184u"J"

I'm happy to add Torr though. Also note that atm is already in the package in the constants submodule - though perhaps it could be "aliased" to the units module somehow. But the current code in this PR will error since atm is already a symbol.

I suppose, when it comes down to it, the main thing keeping me from just registering the unit myself and going forward is an aesthetic issue; I like that having all my units wrapped in u"" makes it easy to pick them out and doesn't clutter the namespace too much. But if it's just for calories, I can register the unit within my base package (which gets imported by my other projects), export it, and just use the cal symbol instead of u"cal". That's not too onerous.

(I'm guessing that the scoping mechanism built into @u_str couldn't be adjusted to recognize that a symbol cal has been registered in a package and exported, and pick it up? That sounds like a complicated procedure.)

I appreciate your willingness to talk me through this--after thinking it through, I can live with cal being its own symbol that lives outside of u"".

Oops I just remembered, atm is already in the package but is listed in the constants module:

@register_constant atm 101325 * U.Pa

I'm used to thinking of atm as a unit of pressure, rather than a measured constant, since actual atmospheric pressure varies so much from that single value. But for the sake of moving this along I'm happy to leave it in the constants module if that's what you prefer.

Since I'm adding mTorr (which would be equivalent to umHg), if we add mmHg, should I add it as mHg with prefixes u and m allowed? I do hear elder statesmen in my field talk about "microns" meaning "umHg" sometimes, but mTorr is probably enough to cover that unit anyway.

src/units.jl Outdated Show resolved Hide resolved
Copy link
Member

@MilesCranmer MilesCranmer left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@MilesCranmer MilesCranmer enabled auto-merge (squash) November 7, 2024 18:49
@MilesCranmer MilesCranmer merged commit a412ab1 into SymbolicML:main Nov 7, 2024
5 checks passed
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.

2 participants