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

feat: implement GET DIRECT SUBSCRIPT for CSV #79

Merged
merged 9 commits into from
Jul 23, 2021

Conversation

ToddFincannon
Copy link
Collaborator

@ToddFincannon ToddFincannon commented Jul 10, 2021

Fixes #77

Implement GET DIRECT SUBSCRIPT in the subscript range reader by reading a CSV file with subscript indices named in the function

Base automatically changed from todd/75-antlr-upgrade to develop July 15, 2021 20:47
Copy link
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.

@ToddFincannon I got the latest from develop merged into this branch, and I set the antlr4-vensim dependency to use the latest from your todd/3-get-direct-subscript branch. So now the build proceeds, but the directsubs test fails here:
https://github.com/climateinteractive/SDEverywhere/runs/3080874108?check_suite_focus=true#step:5:62

It looks like there needs to be some code that resolves the location of the csv file relative to the mdl file, is that right? Can you take a look?

The rest of the fix looks good to me, so I think this will be ready to be merged once that test failure is resolved.

@chrispcampbell chrispcampbell changed the title feat: implement GET DIRECT SUBSCRIPT for CSV (#77) feat: implement GET DIRECT SUBSCRIPT for CSV Jul 15, 2021
@ToddFincannon
Copy link
Collaborator Author

Quite right — I realized that and fixed the modeltests script in the following branch #78. Maybe we need to back port that fix to the #77 branch.

@chrispcampbell
Copy link
Contributor

Quite right — I realized that and fixed the modeltests script in the following branch #78. Maybe we need to back port that fix to the #77 branch.

Ah, I hadn't looked at that branch yet, but I see that change now. Seems more like a workaround though if users have to call sde from a particular directory (i.e., from the same directory as the model) for the csv file path to be resolved correctly. How hard do you think it would be to make it so the csv file is resolved relative to the mdl file (assuming that's how Vensim resolves it)?

@ToddFincannon
Copy link
Collaborator Author

I did the back port of the modeltests script from #78, but now the arrays_cname model test is broken. I think it's unrelated to the change in the test script. Will investigate further next week.

@chrispcampbell
Copy link
Contributor

I did the back port of the modeltests script from #78, but now the arrays_cname model test is broken. I think it's unrelated to the change in the test script. Will investigate further next week.

@ToddFincannon I'm not seeing the test failure in arrays_cname that you mention. That said, I would still prefer to not change the test script (i.e., should back out the change in 245bc48); it doesn't seem right to have to call sde from a particular cwd for GET DIRECT SUBSCRIPT to work correctly.

Can we instead change SDE to resolve the path of the csv file relative to the mdl file? I sketched it out incompletely in this gist; would something like this work?

@ToddFincannon
Copy link
Collaborator Author

I backed out the modeltests change, and will do the same for #78 and succeeding branches. I implemented the code in your gist to resolve the CSV pathname relative to the model directory. The arrays_cname test only fails starting from the antlr4-vensim #4 branch. Tests are passing then antlr4-vensim is set to the 3-get-direct-subscript branch. I will fix that separately.

Copy link
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.

Looks good, will merge once the build finishes.

@chrispcampbell chrispcampbell merged commit 51691e7 into develop Jul 23, 2021
@chrispcampbell chrispcampbell deleted the todd/77-get-direct-subscript branch July 23, 2021 22:58
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 GET DIRECT SUBSCRIPT
2 participants