Remove "clamping" from balance checks #416

Activity

1. priyanshusky0 commented on Oct 24, 2026

   

   priyanshusky0

   on Oct 24, 2026

   Contributor

   More actions

   Hi @Alomir, I'd like to work on this if it's available. Here's my plan:

   balance.h / balance.c

   • Remove clampedC, clampedN, finalC and finalN from BalanceTracker, along with their initialization in initBalanceTracker().
   • Remove updateBalanceTrackerPostClamp(), and move the system input/output calculation into updateBalanceTrackerPostUpdate(), without adding clamped mass to inputs.
   • In checkBalance(), compute the pool delta as postTotal - preTotal, and drop clamped from the log messages.

   sipnet.c:updatePoolsAndBalance()

   • New order: updateBalanceTrackerPreUpdate(), then the pool updates, then checkForMortality(), then ensureNonNegativeStocks(), then updateBalanceTrackerPostUpdate(), then checkBalance(). So PostUpdate takes the place of the old PostClamp call.

   Tests

   • Remove the clampedC and clampedN assertions in testCompleteHarvest.c.
   • Check the direct PreUpdate/PostUpdate usage in testCompleteHarvest.c and testEventHarvest.c.
   • Update testBalance.c if needed.
   • Run all unit and smoke tests.

   Questions:

   1. Overlap with [#411] Make carbon and nitrogen balance failures fatal #414: my PR [#411] Make carbon and nitrogen balance failures fatal #414 (fatal balance check) touches the same code, and its error message and test still use finalC and clampedC. Would you prefer that I build this on top of [#411] Make carbon and nitrogen balance failures fatal #414 after it merges, or do this first and rebase [#411] Make carbon and nitrogen balance failures fatal #414 onto it?
   2. Small values reset to zero: ensureNonNegative() still zeroes values in [-EPS, 0). Since the post-update totals would now be taken after that step, those tiny adjustments will appear in the balance delta. I expect them to stay under tolerance, but I'll confirm with the smoke tests. Let me know if you'd rather handle that differently.
   3. Error message bug: the logError call in ensureNonNegative() has a format mismatch. It passes label, *var, year, ... to "%s year %d ...", so the value isn't printed correctly. I can fix that here or in a separate PR, whichever you prefer.
2. Alomir commented on Oct 27, 2026

   

   Alomir

   on Oct 27, 2026

   CollaboratorAuthor

   More actions

   Yes definitely, you're the right person for this. As for your questions:

   1. I would say let's get [#411] Make carbon and nitrogen balance failures fatal #414 in first, I only had one tiny change suggestion for it. This new PR should build onto that, either as a sub-PR or from master if you want to wait a short while for [#411] Make carbon and nitrogen balance failures fatal #414 to go in (for practical purposes, I guess those are really the same thing!)
   2. This is a good question; any one instance of a tiny adjustment would be fine, but it is would be possible for several to add up to more than -EPS. My first thought would be to take into account the summation nature of that final total, and use, say, -10*EPS as the final tolerance. Still plenty small to be believable as noise, but big enough to allow for accumulation. Let me know if you have another idea.
   3. I think this was reported in [BUG] Format string specifier mismatch in ensureNonNegative() causes undefined behavior #406, which has been assigned to @Labreo . No need to fix here!
3. Alomir

   assigned

   priyanshusky0

   on Oct 27, 2026
4. priyanshusky0 commented on Oct 28, 2026

   

   priyanshusky0

   on Oct 28, 2026

   Contributor

   More actions

   @Alomir

   Opened #425 for this. It's built on top of #414, so that commit shows there until #414 is merged.

   I went with 10 * EPS as the balance tolerance as you suggested. I applied it in both directions, since the zeroing in ensureNonNegative only ever adds mass, so the sign shouldn't matter. Happy to change it if you meant something else.

   All unit and smoke tests pass.
5. priyanshusky0

   added a commit that references this issue on Nov 5, 2026

   Remove clamping from balance checks (PecanProject#416)

   ...

   c3d5b47