refactor: replace Unitful with FlexUnits#165
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the Unitful dependency with FlexUnits, updating the internal unit module to FlexUnitsBatsrus and adjusting documentation, examples, and extensions accordingly. Type constraints were also added to core data structures in src/type.jl. Feedback focuses on the new unit implementation, noting that changing physical constants from values to functions is a breaking change. Additionally, the reviewer recommended addressing performance issues related to Ref{Any} usage, removing redundant logic in getunit, and refactoring duplicated calculations for the Rydberg constant.
| me() = _me[] | ||
| mp() = _mp[] | ||
| q() = _q[] | ||
| k() = _k[] | ||
| μ0() = _μ0[] | ||
| ε0() = _ε0[] | ||
| R() = _R[] | ||
| Ry() = _Ry[] |
There was a problem hiding this comment.
The constants me, mp, q, etc., have been changed from values (in the previous Unitful implementation) to functions. This is a breaking change for any existing code or examples (like examples/vdf.jl) that expect these to be values in arithmetic expressions. If the intention is to maintain the same API, consider using a different approach or ensure that all internal and external usages are updated to call these as functions (e.g., me()).
| const _me = Ref{Any}() | ||
| const _mp = Ref{Any}() | ||
| const _q = Ref{Any}() | ||
| const _k = Ref{Any}() | ||
| const _μ0 = Ref{Any}() | ||
| const _ε0 = Ref{Any}() | ||
| const _R = Ref{Any}() | ||
| const _Ry = Ref{Any}() |
There was a problem hiding this comment.
Using Ref{Any} for global constants that are initialized at runtime is a performance anti-pattern in Julia. It forces dynamic dispatch every time these constants are accessed. Since these values are physical constants with units (likely FlexUnits.Quantity), using a more specific type or adding type assertions in the accessor functions would significantly improve performance in hot loops.
| register_unit("Ry" => 13.605693122994 * 1.602176634e-19 * qparse("J")) | ||
|
|
||
| # densities | ||
| # amucc: amu/cc | ||
| register_unit("amucc" => qparse("u/cm^3")) | ||
|
|
||
| # velocities | ||
| # kms: km/s | ||
| register_unit("kms" => qparse("km/s")) | ||
|
|
||
| # current densities | ||
| # ampm2: μA/m² | ||
| register_unit("ampm2" => qparse("μA/m^2")) | ||
|
|
||
| # Others | ||
| # tm2: nT/m² | ||
| register_unit("tm2" => qparse("nT/m^2")) | ||
| # vm2: V/m² | ||
| register_unit("vm2" => qparse("V/m^2")) | ||
|
|
||
| # Physical constants | ||
| _me[] = 9.1093837015e-31 * qparse("kg") | ||
| _mp[] = 1.67262192369e-27 * qparse("kg") | ||
| _q[] = 1.602176634e-19 * qparse("C") | ||
| _k[] = 1.380649e-23 * qparse("J/K") | ||
| _μ0[] = 4π * 1e-7 * qparse("T*m/A") | ||
| _ε0[] = 8.8541878128e-12 * qparse("F/m") | ||
| _R[] = 8.31446261815324 * qparse("J/(mol*K)") | ||
| _Ry[] = 13.605693122994 * 1.602176634e-19 * qparse("J") |
There was a problem hiding this comment.
The calculation for the Rydberg constant (Ry) is duplicated. It is calculated once for unit registration and again for the constant storage. It's better to calculate it once and reuse the value.
ry_val = 13.605693122994 * 1.602176634e-19 * qparse("J")
# Ry: Rydberg
register_unit("Ry" => ry_val)
# ... (other registrations)
_Ry[] = ry_val| typeunit = bd.head.headline == "PLANETARY" ? "PLANETARY" : | ||
| _get_typeunit(bd.head.headline) |
There was a problem hiding this comment.
The explicit check for "PLANETARY" here is redundant because _get_typeunit(bd.head.headline) already handles this case and returns "PLANETARY".
typeunit = _get_typeunit(bd.head.headline)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #165 +/- ##
==========================================
- Coverage 83.05% 82.97% -0.09%
==========================================
Files 21 21
Lines 4155 4199 +44
==========================================
+ Hits 3451 3484 +33
- Misses 704 715 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
1 participant
This is an experiment on switching from Unitful to FluxUnits.