Fixing bind mapping with multiple models #90
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
Conversation
There was a problem hiding this comment.
Hi @phajy this looks good! I would ask for a few more test cases though, specifically
- 3 models, 1 bound parameter
- 3 models (2 the same, 1 different), bind one parameter between the two
- 2 models, both different, bind a parameter that is only in one model (check it does no-ops okay)
Have you tested it on your use case? Might be worth adding an integration test somewhere too, but maybe that's for a new PR. Let's get this one merged first.
|
I've added some test cases. However, test case 3 suggested above (the no-op case) does not work. If you try to bind a parameter that is not in one of the models Any suggestions? |
|
Well noticed, I think we should address that in a new PR though. This one has the test suite passing so I think it's good to merge. |
|
I should get rid of the debug print statements - do you want me to create a new PR for that? |
|
I think it's fine just to do it in this PR. If you could add one more commit removing the debug statements 👍 |
|
Done! Print statements removed. Note, however, that the test suite is only passing because I have commented out the code that tests
because it does not work yet. See #90 (comment) above. |
|
Sorry, accidentally closed the PR. Happy for you to merge. |
This PR addresses #89 regarding a problem with
bindif there are multiple models and datasets.Once a parameter has been bound to another parameter it must relinquish the parameter number it had before being bound. This means that there is now a gap in the contiguous list of parameter numbers. Larger parameter numbers then need to be shuffled down by one to fill this gap, resulting in a contiguous list of parameters. The problem was that all later parameters in the lit were being decremented by 1 regardless of whether they were larger or not.
A test case has been added to check that
bindworks as anticipated for multiple models and datasets. Further tests could usefully be added.Finally, there are lots of debugging
printlnstatements that can be removed once we're satisfied the code is working properly.