Skip to content

feat: add support for Vensim's ALLOCATE BY PRIORITY function#809

Merged
chrispcampbell merged 10 commits intoclimateinteractive:mainfrom
NiharikaHari:feat/allocate-by-priority
Apr 28, 2026
Merged

feat: add support for Vensim's ALLOCATE BY PRIORITY function#809
chrispcampbell merged 10 commits intoclimateinteractive:mainfrom
NiharikaHari:feat/allocate-by-priority

Conversation

@NiharikaHari
Copy link
Copy Markdown
Contributor

@NiharikaHari NiharikaHari commented Apr 20, 2026

Fixes #792

Feature: Allocate by priority #792

  • Adds support for Vensim’s allocate by priority function in the C runtime.
  • Mirrors the structure and style of the existing allocate_available implementation for consistency.
  • Includes Vensim’s example model (with .dat file) under models/ for validation and comparison of outputs.
  • Note: JS support is not implemented yet.

@chrispcampbell
Copy link
Copy Markdown
Contributor

@NiharikaHari: Thank you for your contribution! I have some high priority tasks to attend to for the next couple days, but I will try my best to review your work by Thursday.

@chrispcampbell chrispcampbell self-requested a review April 24, 2026 00:29
Copy link
Copy Markdown
Contributor

@chrispcampbell chrispcampbell left a comment

Choose a reason for hiding this comment

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

Thank you @NiharikaHari for taking the time to contribute your work on this new function!

Most of my comments below are nitpicky things about code style. I haven't read the logic too closely for the _ALLOCATE_BY_PRIORITY C runtime function, but as long as it produces equivalent results to Vensim (and I can see that it does since the tests are passing), then I will trust that the logic works.

The test_js job is failing; that job does more than just run JS-related tests, it also runs the JS-level integration tests that exercise both C/Wasm and JS code gen, and there are Emscripten warnings flagged here (see build log) that will need to be resolved.

Also, please add at least one new test in gen-equation-c-from-vensim.spec.ts to verify that code gen works as expected. You can follow the patterns used in the existing should work for ALLOCATE AVAILABLE function (1D LHS, ...) test. Add your new test just after the last ALLOCATE AVAILABLE test in that file.

Comment thread packages/cli/src/c/vensim.c Outdated
Comment thread packages/cli/src/c/vensim.c Outdated
Comment thread packages/cli/src/c/vensim.c Outdated
Comment thread packages/cli/src/c/vensim.c Outdated
Comment thread packages/cli/src/c/vensim.c Outdated
Comment thread packages/cli/src/c/vensim.c Outdated
Comment thread packages/cli/src/c/vensim.c Outdated
Comment thread packages/cli/src/c/vensim.c Outdated
Comment thread packages/cli/src/c/vensim.c Outdated
Comment thread packages/compile/src/model/read-equations.js Outdated
@NiharikaHari
Copy link
Copy Markdown
Contributor Author

Thank you @chrispcampbell for taking the time to review this. I'll be able to work on these changes next week. The logic in _ALLOCATE_BY_PRIORITY C runtime function mirrors the algorithm used in PySD.

@travisfranck
Copy link
Copy Markdown
Collaborator

@NiharikaHari This is a useful addition to SDE. Thank you for contributing! Does you model run now?

@NiharikaHari
Copy link
Copy Markdown
Contributor Author

@travisfranck Yes, it does! Outside of this I'm doing some janky changes around setConstants and setLookups to read from CIN files and CSV respectively in C runtime. Excited to integrate SDE into our pipeline :)

@NiharikaHari
Copy link
Copy Markdown
Contributor Author

@chrispcampbell I have made the requested changes. The failures in test_js look like they were because of the printf statements, which I have removed now.

@travisfranck
Copy link
Copy Markdown
Collaborator

@travisfranck Yes, it does! Outside of this I'm doing some janky changes around setConstants and setLookups to read from CIN files and CSV respectively in C runtime. Excited to integrate SDE into our pipeline :)

Congrats! Again, thank you for contributing to SDE.

@chrispcampbell
Copy link
Copy Markdown
Contributor

@NiharikaHari:

The failures in test_js look like they were because of the printf statements, which I have removed now.

You're correct that removing the printf statements resolved the first set of errors, but there was one more at the bottom, so the build was still failing:
image

We need to update the modeltests script so that it skips your new model when running tests for the JS target, just like we did for the allocate model. I made the change already and pushed to your branch.

Copy link
Copy Markdown
Contributor

@chrispcampbell chrispcampbell left a comment

Choose a reason for hiding this comment

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

I verified that you addressed all of the review comments. All good now, and the builds are successful. I will merge shortly and will publish new versions of the @sdeverywhere packages in a bit. Thanks again for contributing!

@chrispcampbell chrispcampbell merged commit 197b261 into climateinteractive:main Apr 28, 2026
3 checks passed
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.

Implement Vensim's ALLOCATE BY PRIORITY

3 participants