Skip to content

Replace manual unit conversions with ud_convert() for consistency across models#3719

Merged
infotroph merged 59 commits intoPecanProject:developfrom
ayushman1210:fix
Mar 7, 2026
Merged

Replace manual unit conversions with ud_convert() for consistency across models#3719
infotroph merged 59 commits intoPecanProject:developfrom
ayushman1210:fix

Conversation

@ayushman1210
Copy link
Contributor

@ayushman1210 ayushman1210 commented Dec 14, 2025

Description

This PR standardizes dimensional unit conversions by replacing manual arithmetic with the centralized helper function PEcAn.utils::ud_convert() where appropriate. This reduces maintenance burden, improves readability, and minimizes the risk of unit conversion errors.
The changes focus strictly on dimensional unit conversions (mass, time, energy, area) and intentionally exclude model-specific scaling, biological constants, and timestep-dependent logic, per project guidance.

Summary of Changes

  • models/sipnet/R/model2netcdf.SIPNET.R
  • Replaced manual mass unit conversions (gC → kgC) with ud_convert()
  • Retained division by timestep.s for flux variables, since SIPNET outputs per-timestep accumulations (model-specific and required)
  • Left biological constants and model-specific scalings unchanged (e.g., SLA, carbon fractions, cm→mm water conversions)
  • models/dalec/R/model2netcdf.DALEC.R
  • Replaced manual conversions (* 0.001, / 86400) with ud_convert()
  • Removed redundant time division where ud_convert() already converts from daily to per-second units
  • Ensured fluxes are consistently written as kgC m⁻² s⁻¹ and pools as kgC m⁻²
  • models/gday/R/model2netcdf.GDAY.R
  • Replaced manual Mg/ha ↔ SI conversions with ud_convert()
  • Removed extra per-second scaling where ud_convert() already performs year → second conversion
  • Kept pool variables as state quantities without time division
  • models/sipnet/R/write_restart.SIPNET.R
  • Replaced manual mass conversions with ud_convert()
  • Removed redundant conversion factors while preserving model-specific logic
  • models/stics/R/met2model.STICS.R
  • Replaced precipitation conversion (kg m⁻² s⁻¹ → mm d⁻¹) with ud_convert()
  • Left radiation and other physical constants unchanged where conversion is not purely dimensional

NOT CHANGED

  • Biological / stoichiometric constants
  • Model-specific timestep logic
  • Physical constants mixed with unit conversions
  • Plot- or model-convention scaling

Fixes - #3712

Copy link
Member

@infotroph infotroph 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 working on this! Can you describe how you tested these changes? I ask because I see a few issues here that should have produced obvious error messages in a test run. The issues themselves are minor and will be easy to fix, but since unit conversions can be surprisingly tricky it's important to know at review time that someone has actually run the code and looked at its output.

@ayushman1210
Copy link
Contributor Author

hi @infotroph Thanks for the review and for pointing this out.
You’re right — I should have been clearer about testing. In this case, I did not run a full test or execute all code paths after making the changes. My checks were based on static reasoning and spot inspection, and a proper run would indeed have surfaced the issues you noticed.
I understand why this is especially important for unit conversions, and I agree that changes like this should always be validated by running the code and inspecting the output. I’ll fix the issues shortly and will make sure to:

  • run the code locally,
  • verify the relevant outputs,
  • and explicitly describe the testing performed in the follow-up.

Thanks for the reminder !!

@ayushman1210
Copy link
Contributor Author

Hi @infotroph
Just a gentle follow-up regarding the PR [#3719 ]
Whenever you get a chance, I’d really appreciate a review.
Thanks for your time!

ayushman1210 and others added 11 commits December 26, 2025 15:36
Co-authored-by: Chris Black <chris@ckblack.org>
Co-authored-by: Chris Black <chris@ckblack.org>
Co-authored-by: Chris Black <chris@ckblack.org>
Co-authored-by: Chris Black <chris@ckblack.org>
Co-authored-by: Chris Black <chris@ckblack.org>
Co-authored-by: Chris Black <chris@ckblack.org>
Co-authored-by: Chris Black <chris@ckblack.org>
@ayushman1210
Copy link
Contributor Author

hey @infotroph sorry for the late response i have exams and college related work.
i have made changes as per your suggestion please review this pr
thanks !!

@infotroph
Copy link
Member

@ayushman1210 The hopefully-last step before merging is to debug the remaining CI / sipnet job failure. I don't see any obvious clues in the log messages, so I suggest trying to reproduce it in a local Sipnet run -- maybe by stepping through the Demo 1 notebook (documentation/tutorials/Demo1_Basic_Run/run_pecan.qmd) after installing the code from this branch -- where you can inspect the outputs.

@infotroph
Copy link
Member

@ayushman1210 I saw the issue, pushed a fix, and will merge as soon as I see tests pass -- apologies for not waiting, but I'm eager to merge this ahead of some other work. Thanks for getting this done!

@infotroph infotroph merged commit 716e281 into PecanProject:develop Mar 7, 2026
19 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants