Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add new implementation of read and code gen phases based on parse package #413

Merged
merged 138 commits into from
Jan 8, 2024

Conversation

chrispcampbell
Copy link
Contributor

@chrispcampbell chrispcampbell commented Dec 5, 2023

Fixes #412

@ToddFincannonEI: This PR adds the new reader and code gen implementations that I described earlier. See issue for more info. Let me know if you'd like more details on anything.

A couple notes about compatibility:

  • Both old and new code are passing 100% of the unit tests in the compile package (the .spec.ts files I added recently).

  • I added a new "compare" mode to the existing modeltests script. So now if you do pnpm test:c-int compare [model] it will run through all models (or the specified one), and for each one it builds the model twice, once with the old and once with the new, and then compares the generated code (using git diff -w to ignore minor whitespace differences). The new code passes the comparison test for all models except for 3 that have minor differences in code gen due to how parentheses are handled or how the IF THEN ELSE are reduced, but otherwise they produce the correct model outputs.

  • I tested En-ROADS and the new code produces exactly the same model outputs. There are only some slight differences in the generated code due to how parens are reduced and a few cases where an IF THEN ELSE reduces to a constant (in the old code it would be treated as an aux, in the new code it gets treated as a const, but otherwise they behave the same).

  • I also tested C-ROADS and there's one small issue with the new code, which I'll either fix on this branch or save for later. (I forgot to finish implementing support for specialSeparationDims before I filed the PR. I have since pushed a fix to this branch along with a test since we didn't already have coverage for it.)

  • If you want to test EPS with the new code, use SDE_NONPUBLIC_USE_NEW_PARSE=1 sde ....

  • If you want a better apples-to-apples comparison of the generated code between old and new, I'd recommend setting SDE_CODE_GEN_CHUNK_SIZE=0 which will disable the function chunking in code gen that makes diffs harder to interpret.

  • If it's helpful, you can also disable the IF THEN ELSE reduction optimization for both old and new implementations using SDE_NONPUBLIC_REDUCE_VARIABLES=0.

…es because the kind tag will remain "number")
…bles similar to the optimization in the legacy reader
…not have been set for the referenced variable
…when original subs/dims are not already in normalized order
@chrispcampbell
Copy link
Contributor Author

For the record, Todd tested the latest version of the EPS model against this branch and found that the new parsing + code gen paths produce results that are compatible with the old, so he is comfortable with merging this PR. I will merge it to main shortly.

@chrispcampbell chrispcampbell merged commit 5538a4f into main Jan 8, 2024
6 checks passed
@chrispcampbell chrispcampbell deleted the chris/parse branch January 8, 2024 19:45
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.

Add new implementation of read and code gen phases based on parse package
1 participant