search

OSeMOSYS / OSeMOSYS_GNU_MathProg

The GNU MathProg implementation of OSeMOSYS
Apache License 2.0
9 stars 14 forks source link

Addition of conditional statements for defaults - use -1 instead of 99999. #30

Closed tniet closed 4 years ago

tniet commented 4 years ago

This is to address issue #26 and #13. (I'm not sure how to pull in references to those issues with this pull request.)

tniet commented 4 years ago

Hi @willu47 - This build crashes after 10 minutes but there's no output in the Travis CI that indicates any error message or what command did not exit. When I run the model on my computer I get the same output for both models. How can I determine the command that Travis CI ran that didn't produce any output?

willu47 commented 4 years ago

Hi @tniet - the issue here is that the test with CBC is being run. Now, again, because the changes to the tests/utopia.txt file default values have affected the long version of the code, the CBC solve is going bananas and never ends. That's why the tests hang.

In general I think we should make changes to both the long and short versions of the code. This is because we want users to seamlessly move from using the long code to the short code. If they have to update their entire datafile so as to use the short code, it will be a real turn off...

tniet commented 4 years ago

Hi @willu47

In general I think we should make changes to both the long and short versions of the code. This is because we want users to seamlessly move from using the long code to the short code. If they have to update their entire datafile so as to use the short code, it will be a real turn off...

I can see the argument for that, but I also think that no one who uses the model for serious modelling should be using the long code. It reduces the efficiency of the solve and provides no real benefit. And, since all serious modelers (should) use the short code, it is natural that improvements and updates are suggested for the short code. Not accepting any changes/updates if they are not in both the short code and long code may reduce the ability to incorporate changes.

Not sure if there's an easy solution, but it's definitely something to keep in mind. Maybe there is a way for Travis CI checks to indicate that the short code changes pass but that the long code doesn't, rather than just a general fail?

willu47 commented 4 years ago

@tniet - the changes I have made in commit 8bcd3c8 allow the tests to run individually. If you like, we can split them into separate files, so that it is clearer which tests are for the long and which are for the short code. However if...

...no one who uses the model for serious modelling should be using the long code...

...then we should remove the long version of the code. It just adds an extra burden on maintenance and is confusing to users.

tniet commented 4 years ago

Hey @willu47 - Updated the long code for this as well, and now it passes both checks, as expected. When merging I had to remove a couple of your changes but they were essentially the same as my changes anyway. I'll comment on the long code vs. short code in the issue you created for that purpose.

willu47 commented 4 years ago

Great. If the checks are passing, then we can merge this. Now, I'm guessing the changes are not backwards compatible right because different default values are required in the data files?

tniet commented 4 years ago

The changes should be backwards compatible. A default of -1 will not create the constraint, but a default of 999999 will create the constraint the same way it currently does. So, overall, I don't see a compatibility issue. Using the default 9999999 will not gain the minor improvements in the run speed of not creating the extra equations, but in most cases that's likely to be a minor issue.