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 the units: parts per hundred thousand (pcm), million (ppm), billion (ppb), trillion (ppt), and quadrillion (ppq) #699

Merged
merged 5 commits into from
Nov 28, 2023

Conversation

eliascarv
Copy link
Contributor

@eliascarv eliascarv commented Nov 21, 2023

@juliohm
Copy link

juliohm commented Nov 22, 2023

Appreciate if someone can review and merge this. It is a very useful unit for chemical analysis.

@cstjean
Copy link
Contributor

cstjean commented Nov 22, 2023

We define our own ppm/ppb, but it's its own dimension, which feels better than having 3ppm == 0.000003.

@eliascarv
Copy link
Contributor Author

eliascarv commented Nov 23, 2023

Sorry @cstjean, I didn't understand your comment, could you explain?

src/pkgdefaults.jl Outdated Show resolved Hide resolved
src/pkgdefaults.jl Outdated Show resolved Hide resolved
src/pkgdefaults.jl Outdated Show resolved Hide resolved
Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>
@eliascarv eliascarv requested a review from sostock November 23, 2023 13:28
@cstjean
Copy link
Contributor

cstjean commented Nov 23, 2023

We have

@dimension PPer     "PartsPer"          PartsPer
@refunit ppb        "ppb"               PartsPerBillion     PPer         false
@unit ppm           "ppm"               PPM                 1000ppb      false

(perhaps that's not super clean, but eh)

I'm not sure what's more appropriate for Unitful, but for us, in terms of "units exist to handle conversion code and prevent errors", making it its own dimension is the right call, even if it means having to add a /ppm division here and there.

Are there circumstances where you'd want 1ppm to be equal to 0.000001? For us, it's no, but I can definitely see that it could be true in some calculations, and in that case, this PR makes more sense than our code. Dividing by 1000000ppm sounds a bit awkward for that conversion.

What do you think? For us it doesn't matter (I think?) as we can override the units. But obviously since changing it afterwards is difficult, so I thought I'd raise the issue at least.

@sostock
Copy link
Collaborator

sostock commented Nov 24, 2023

Percent, Permille, and Permyriad are dimensionless, so adding a dimension only for ppm, ppb, ppb, and ppq would be highly inconsistent. And changing Percent, Permille, and Permyriad to use that new dimension would be breaking.

SI also defines 1% == 0.01, and it doesn’t seem controversial to me.

@sostock
Copy link
Collaborator

sostock commented Nov 24, 2023

Reference: Parts-per notation - Wikipedia

The Wikipedia page also mentions pcm (per cent mille). Does it make sense to add that as well? Then we would have all units listed on the page.

@sostock sostock added new units adding new units/dimensions/constants to this package dimensionless dimensionless quantities/units (mm/m, rad, …) and their interaction with plain numbers labels Nov 24, 2023
@eliascarv
Copy link
Contributor Author

@sostock, added.

@eliascarv eliascarv changed the title Add the units: parts per million, billion, trillion, and quadrillion Add the units: parts per hundred thousand, million, billion, trillion, and quadrillion Nov 24, 2023
@eliascarv eliascarv changed the title Add the units: parts per hundred thousand, million, billion, trillion, and quadrillion Add the units: parts per hundred thousand (pcm), million (ppm), billion (ppb), trillion (ppt), and quadrillion (ppq) Nov 24, 2023
@juliohm
Copy link

juliohm commented Nov 24, 2023

Thank you all for the input. We are looking forward to seeing it merged and released. We already have some downstream applications that will benefit from these new units.

@eliascarv
Copy link
Contributor Author

@sostock, the PR is ready. Can you check if it's ready to merge?

@sostock sostock merged commit d23bbac into PainterQubits:master Nov 28, 2023
12 of 13 checks passed
@eliascarv eliascarv deleted the parts-per branch November 28, 2023 19:08
@eliascarv
Copy link
Contributor Author

@sostock, could we have a patch release with this PR?

@sostock
Copy link
Collaborator

sostock commented Nov 28, 2023

No, this is a feature, so it cannot go into a patch release. I will release it as v1.19.0 shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dimensionless dimensionless quantities/units (mm/m, rad, …) and their interaction with plain numbers new units adding new units/dimensions/constants to this package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add parts per million (ppm) unit
4 participants