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 atomic units of energy, mass, length, electric charge and current. #324

Merged

Conversation

crystal-growth
Copy link
Contributor

Adding

  • Dalton (unified atomic mass unit = 1/12 * (mass of C12))
  • Hartree (atomic unit of energy, ~27.2 eV)
  • atomic unit of length (Bohr radius, 5.291 772 109 03 x 10-11 m )
  • atomic unit of charge (absolute charge of electron, 1.602_176_634 x 10^-19 C)

Copy link
Owner

@iliekturtles iliekturtles left a comment

Choose a reason for hiding this comment

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

I didn't get very far in this review. Comments about using "elementary charge" which seems more common than "atomic units"? Especially since the 2019 redefinition which uses "elementary charge" to define the ampere.

I included a possible change that has both elementary charge and atomic unit of charge.

src/si/electric_charge.rs Outdated Show resolved Hide resolved
@crystal-growth
Copy link
Contributor Author

Thanks, completely agree with elementary charge!
Squashed and rebased.

Copy link
Owner

@iliekturtles iliekturtles left a comment

Choose a reason for hiding this comment

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

Couple more questions about some of the constants that seem to be used as units. Otherwise close to merging!

@@ -98,6 +102,8 @@ mod tests {
test::<i::abampere, t::second, q::abcoulomb>();
test::<i::statampere, t::second, q::statcoulomb>();

test::<i::atomic_unit_of_charge_per_second, t::second, q::atomic_unit_of_charge>();

Copy link
Owner

Choose a reason for hiding this comment

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

Github won't let me use the full context for the changes, so I had to paste a diff below.

Put the elementary charge and atomic unit of charge tests in the same order as the unit definitions. For elementary charge possibly use elementary charge per second (see electric current review) or fallback to atomic unit of charge per second.

diff --git a/src/si/electric_charge.rs b/src/si/electric_charge.rs
index c749ec6..ea7406b 100644
--- a/src/si/electric_charge.rs
+++ b/src/si/electric_charge.rs
@@ -98,12 +98,12 @@ mod tests {
             test::<i::zeptoampere, t::second, q::zeptocoulomb>();
             test::<i::yoctoampere, t::second, q::yoctocoulomb>();
 
+            test::<i::elementary_charge_per_second, t::second, q::elementary_charge>();
+            test::<i::atomic_unit_of_charge_per_second, t::second, q::atomic_unit_of_charge>();
             test::<i::ampere, t::hour, q::ampere_hour>();
             test::<i::abampere, t::second, q::abcoulomb>();
             test::<i::statampere, t::second, q::statcoulomb>();
 
-            test::<i::atomic_unit_of_charge_per_second, t::second, q::atomic_unit_of_charge>();
-
             fn test<I: i::Conversion<V>, T: t::Conversion<V>, Q: q::Conversion<V>>() {
                 Test::assert_approx_eq(&ElectricCharge::new::<Q>(V::one()),
                     &(ElectricCurrent::new::<I>(V::one()) * Time::new::<T>(V::one())));

src/si/electric_current.rs Outdated Show resolved Hide resolved
src/si/energy.rs Outdated Show resolved Hide resolved
src/si/length.rs Outdated Show resolved Hide resolved
src/si/mass.rs Outdated Show resolved Hide resolved
@crystal-growth
Copy link
Contributor Author

Thanks a lot for these suggestions!

  1. Added elementary charge per second. It definitely should exist.
  2. Agree with Eₕ, it's more common, and better stresses that it is Energy
  3. I'm not sure if Bohr radius is an "official" unit, but it is very convenient to use it for describing hydrogen-like states in semiconductor physics:
    let bohr_radius_of_shallow_donor_in_silicon = Length::new::<bohr_radius>(some_nondimentional_scaling_factor);
    so I fully agree with adding it together with a.u. of length.

Co-authored-by: Mike Boutin <mike.boutin@gmail.com>
@iliekturtles iliekturtles merged commit f8caf07 into iliekturtles:master Aug 23, 2022
@iliekturtles
Copy link
Owner

Thanks so much for this PR! I just returned from vacation and will get back to working on the rest of your PRs.

@crystal-growth
Copy link
Contributor Author

Thank you very much! I apologize for this heap of PRs, and hope it will help improving usability of this great library.

Starting using uom, I found it extremely useful in protecting me from various errors, including using wrong dimension for a given quantity.
However, if a needed quantity is missing -- it becomes much less ergonomic.

Now I'm "dimensionalizing" all my computational scripts, adding PRs for the quantities and units that are missing.
It's a great pleasure to get rid of functions with signatures like:
some_func(atom_radius_in_picometers: f64 , atom_mass_in_amu: f64) -> diffusion_coeffient_in_cm2_s:f64

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