Fix units and unit-conversion factors #2891
Conversation
…le with numba.jit's optimization. Using the constants YR_TO_S and S_TO_YR.
OceanNuclear
changed the title
|
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2891 +/- ##
===========================================
- Coverage 79.98% 74.29% -5.69%
===========================================
Files 222 242 +20
Lines 24759 26861 +2102
===========================================
+ Hits 19804 19957 +153
- Misses 4955 6904 +1949 ☔ View full report in Codecov by Sentry. |
|
(Manually checked and confirmed that this change does not lead to conflict in any other branches) |
|
I'm only starting going through this one. My first thought is that the reduction in the use of the S_TO_YR and YR_TO_S will increase the number of calculations that have to be undertaken. In one instance as the conversion happens within a for loop its a many times increase. Most of the other conversions are fairly sensible but I think there is no need to reduce so drastically the time conversions especially as its so widely used |
|
I thought the number of calculations would be the same across raw_uc(delta_t, "yr", "s")and delta_t * YR_TO_S? As raw_uc does the multiplication under the hood, plus a few other unit lookup calls inside pint. The only place where this might be a significant problem is in the for-loop inside EUDEMOFuelCycleModel.tbreed, but I thought that's not a function needing optimization because it re-calculated the temporary variable |
|
I agree with your second point but your first point Its only in that usecase no need to apply the logic everywhere |
je-cook
force-pushed
the
develop
branch
5 times, most recently
from
5a50acb
to
7574352
Compare
|
* Removed redundant units * Removed all 'years' variable and YR_TO_S variables. Found two possible errors. * Made units more understandable * Undid all the raw_uc conversions in tools.py as they are not compatible with numba.jit's optimization. Using the constants YR_TO_S and S_TO_YR. * Removed as many S_TO_YR and YR_TO_S as possible. * Fixed the problem of missing elementary charge (e) constant in base.constants * Undid all S_TO_YR/YR_TO_S changes * Minor fixes to the YR_TO_S * Added back in C_LIGHT * Made sure all of the units used in neutronics matches the units used in plasma_physics
Linked Issues
Closes #2853 with caveats: YR_TO_S and S_TO_YR cannot be removed completely as bluemira/plasma_physics/tools.py uses them, and cannot be converted to raw_uc there due to numba.jit.
Closes #2857 with caveats: get_species_data needs to be overhauled if this change were to be made, so it has been left unchanged, and may be ultimately solved in issue #2890.
Description
Solves issue #2853 and #2857 simultaneously, so all temperature units are in keV or K (as opposed to a mix of eV/keV/K); and no more conversion multiplies (
*_TO_*) are required.Interface Changes
bluemira.plasma_physics.rules_of_thumb.estimate_loop_voltage now takes in electron temperature in keV instead of eV.
bluemira.plasma_physics.collisions.spitzer_conductivity now takes in electron temperature in keV instead of eV.
Checklist
I confirm that I have completed the following checks:
pytest tests --reactorpre-commit run --from-ref develop --to-ref HEADsphinx-build -W documentation/source documentation/build