Skip to content

Conversation

@sasmith
Copy link
Contributor

@sasmith sasmith commented Nov 26, 2025

Replaced probabilistic resource consumption with deterministic consumption by changing InventoryProbability (float) to InventoryQuantity (int) throughout the codebase.

This is part of removing floats from the simulation. For features that are in active use, I'm aiming to replace floating calculations with integer calculations. For features that aren't in active use, I'm just removing the feature.

https://app.asana.com/1/1209016784099267/project/1209096222434381/task/1211547145607801?focus=true

Copy link
Contributor Author

sasmith commented Nov 26, 2025

@sasmith sasmith marked this pull request as ready for review November 26, 2025 19:04
@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@sasmith sasmith force-pushed the sasmith-11-26-remove_resource_mod branch from 7e1671e to a8e07db Compare November 26, 2025 19:05
@sasmith sasmith force-pushed the sasmith-11-26-remove_fractional_resource_consumption branch 2 times, most recently from 01391f6 to d914588 Compare November 26, 2025 19:23
@sasmith sasmith force-pushed the sasmith-11-26-remove_resource_mod branch from a8e07db to d26fddb Compare November 26, 2025 19:23
@sasmith sasmith force-pushed the sasmith-11-26-remove_fractional_resource_consumption branch from d914588 to bc09d10 Compare November 26, 2025 19:54
@sasmith sasmith force-pushed the sasmith-11-26-remove_resource_mod branch from d26fddb to 6e4a0e7 Compare November 26, 2025 19:54
using InventoryQuantity = uint8_t;
using InventoryProbability = float; // probability for fractional consumption
using InventoryDelta = int16_t; // cover full range of allowed changes (+/-255)
using InventoryDelta = int16_t; // cover full range of allowed changes (+/-255)
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing the InventoryProbability type breaks tests that rely on fractional consumption. Either the tests need to be updated or this type should be kept for backward compatibility.

Spotted by Graphite Agent (based on CI logs)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@sasmith sasmith requested review from daveey and nathants November 26, 2025 20:12
Copy link
Contributor

@nathants nathants left a comment

Choose a reason for hiding this comment

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

Do it. Tests are significantly red, but you are probably either fixing that or fixing it in the branch that merges all these mini prs?

@sasmith sasmith force-pushed the sasmith-11-26-remove_resource_mod branch from 6e4a0e7 to b9e94f5 Compare December 1, 2025 18:44
@sasmith sasmith force-pushed the sasmith-11-26-remove_fractional_resource_consumption branch from bc09d10 to 2e29844 Compare December 1, 2025 18:44
Base automatically changed from sasmith-11-26-remove_resource_mod to main December 1, 2025 19:28
@sasmith sasmith force-pushed the sasmith-11-26-remove_fractional_resource_consumption branch from 2e29844 to 8bbf701 Compare December 1, 2025 19:30
@sasmith sasmith added this pull request to the merge queue Dec 1, 2025
Merged via the queue into main with commit 7b4b3b5 Dec 1, 2025
13 checks passed
@sasmith sasmith deleted the sasmith-11-26-remove_fractional_resource_consumption branch December 1, 2025 19:58
zfogg pushed a commit that referenced this pull request Dec 20, 2025
Replaced probabilistic resource consumption with deterministic
consumption by changing `InventoryProbability` (float) to
`InventoryQuantity` (int) throughout the codebase.

This is part of removing floats from the simulation. For features that
are in active use, I'm aiming to replace floating calculations with
integer calculations. For features that aren't in active use, I'm just
removing the feature.


https://app.asana.com/1/1209016784099267/project/1209096222434381/task/1211547145607801?focus=true

---------

Co-authored-by: S. Alex Smith <sasmith@stem.ai>
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.

3 participants