message_ix icon indicating copy to clipboard operation
message_ix copied to clipboard

Enhancing storage equations by considering `mode` and `lvl_temporal`

Open behnam-zakeri opened this issue 3 years ago • 4 comments

This PR addresses #632 by adding mode to the mapping set of storage technologies (map_tec_storage) and also to storage equations. More details: This change means re-initializing the existing set of map_tec_storage with new index sets. Based on the discussion in #631, this re-initializing happens automatically by modifications in the file models.py, only if the mapping set is empty. If the set is not empty, an exception is raised, telling the user that they need to update this set. Moreover, this PR improves the documentation of storage section to reflect the above changes and more than that, adding required details for each equation.

How to review

  • create a branch from the submitted PR; load an existing scenario (from Westeros, national or global models); clone a new scenario without carrying the solution (using the argument keep_solution=False); solve the cloned scenario again. The cloned scenario should solve without an error, if there is no content in the set map_tec_storage in that scenario. Otherwise, there should be an error asking you to update this set.
  • run test_feature_storage.py from this PR on the main branch; it should fail.
  • read the documentation of the storage section and migration notes, and make sure it's clear and complete.

PR checklist

  • [X] Continuous integration checks all ✅
  • [x] Test is enhanced (test_feature_storage.py) to reflect the new changes.
  • [x] Expanded the documentation.
  • [x] Update release and migration notes.

behnam-zakeri avatar Jul 28 '22 13:07 behnam-zakeri

As discussed in the MESSAGEix meeting today (13 Oct), it will be useful if @khaeru can review the work-around suggested here to re-initialize an existing message_ix set or parameter.

behnam-zakeri avatar Oct 13 '22 12:10 behnam-zakeri

And it will be nice if @julianhunt4 or @adrivinca can review this content-wise, i.e, related to the functionality of storage.

behnam-zakeri avatar Oct 13 '22 12:10 behnam-zakeri

Thanks for the PR, it seems everything works well. Concerning the 'how to review' procedure, I did it. But not sure if I got the second point correctly. Running the test_feature_storage.py on the main branch immediately gives errors because for instance expand_dims is not defined.

I wrote a comment to a point in the GAMs code where the mode component ,m, might have been forgotten.

And last I was wondering if the branch should be rebased to message_ix 3.6, or if, since there are no conflicts, it does not matter.

adrivinca avatar Oct 27 '22 11:10 adrivinca

Thanks @adrivinca. Where is your comment, I can't see that?

behnam-zakeri avatar Oct 27 '22 21:10 behnam-zakeri

@khaeru if you don't have any additional concerns here, I can merge this.

behnam-zakeri avatar Nov 17 '22 15:11 behnam-zakeri

@khaeru if you don't have any additional concerns here, I can merge this.

GitHub says "This branch cannot be rebased due to conflicts". I'll try to rebase, then let's see if tests pass.

khaeru avatar Nov 21 '22 09:11 khaeru

Ready to merge once checks all pass.

Thanks a lot @khaeru for finalizing this.

behnam-zakeri avatar Nov 21 '22 11:11 behnam-zakeri