Introduce GAMS I/O for IXMP4Backend
#558
Conversation
ixmp/backend/io.py
Outdated
|
|
||
|
|
||
| # TODO instead of checking len(indexset.data), make ixmp4 return None if it's empty? | ||
| def _add_indexsets_to_container( |
There was a problem hiding this comment.
These ~6 functions all look very similar to one another. It should be possible to collapse them into one that is not too complex. Should I make an attempt?
There was a problem hiding this comment.
Yes, that's often the case with these because the underlying objects are so similar. I didn't try to make this more abstract yet since I don't know which form we want them to take eventually. For now, I mainly tried to adhere to the dictum that each function should just have one specific task.
I don't think there's any urgency with this, but if you feel inspired, please go ahead :)
There was a problem hiding this comment.
Understood—I'm basically thinking ahead to trying to review and merge this code, and later maintain it. Despite our best efforts it will probably get pretty big, so there will be a lot to 'grok'.
When looking at it from a high level, it can be easier to understand "There's this one function that does [thing] (for a few different kinds of inputs)" than "There are six functions, what do they each do?"
Let me try for a few minutes and see where I get.
There was a problem hiding this comment.
You are much more experienced than me, so I'm happy to go along with your solution, but I'm not sure I agree completely. I think having different functions is costly in two ways: writing more lines of code and maintaining similar functions separately.
However, writing dedicated functions is easier to begin with, I would say, compared to starting with the abstraction, and already done. And maintaining them is only costly if gams changes something that requires identical changes in all functions. And even then, one can easily copy-paste the edits and the tests should catch all places where they need to go. If, on the other hand, gams changes something to only e.g. gt.Set, the abstract function requires additional checks to handle these cases, making it more complex.
Either way, thanks for the commit :)
I think the klass dict is missing Equation and I also tried to improve the type hints, I'll push a fixup commit :)
glatterf42
force-pushed
the
enh/ixmp4-gams-io
branch
from
97e81c4 to
9223f38
Compare
| if isinstance(item, (IndexSet, Scalar)): | ||
| return None | ||
| else: | ||
| return item.indexsets |
There was a problem hiding this comment.
I find this curious: when using return None if isinstance(item, (IndexSet, Scalar)) else item.indexsets, mypy complains that IndexSet and Scalar don't have the attribute .indexsets, but writing it like this is fine. This almost seems like a mypy limitation, maybe it's even worth an issue with them.
There was a problem hiding this comment.
This PR adds the next step of functionality for the new
IXMP4Backend: I/O for GAMS. I can't yet confirm that the functions here cover all functionality, but locally, thescenario.solve()step inmake_dantzig()works.The PR also adds
delete_item()toIXMP4Backendsince that was still needed before getting to the GAMS I/O. TheGAMSModelalways want to record the package versions and remove the sets later on. As it turns out, we don't need to store the package versions in an ixmp4 DB, but I only noticed that after adding the functionality.The whole
make_dantzig()function still fails, however, because it's also trying to add timeseries data, which I will look into next. Handling timeseries data is more on a platform-level, I would say, that's why I want to do that in a separate PR.The commits here are not very clean, either, sorry for that. I figure that's okay since no one else is working on these files at the moment.
How to review
TBD
PR checklist