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

Enhancing storage equations by considering mode and lvl_temporal #633

Merged
merged 19 commits into from
Nov 23, 2022

Conversation

behnam-zakeri
Copy link
Contributor

@behnam-zakeri behnam-zakeri commented Jul 28, 2022

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

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

@behnam-zakeri behnam-zakeri marked this pull request as draft July 28, 2022 13:33
@behnam-zakeri behnam-zakeri self-assigned this Jul 28, 2022
@behnam-zakeri behnam-zakeri added enh New features & functionality timeslice labels Jul 28, 2022
behnam-zakeri added a commit to behnam-zakeri/message_ix that referenced this pull request Jul 28, 2022
@@ -415,6 +419,61 @@ def enforce(scenario):
scenario.remove_set(set_name, existing)
scenario.add_set(set_name, expected)

# Enforcing new indexes for existing set and parameters
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khaeru, following the discussion under #631, I drafted a workflow to help reinitialize some existing sets and parameters here. The code checks if the item is empty before re-initializing, and if not raises an exception. I do believe this can be done much more compact and maybe under a separate method than enforce().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khaeru Resuming working on this, I was wondering if you have time to have a look at the above point. In principle, it's a design issue; how we would like to alert users that they need to re-parameterize some existing sets and parameters in their scenario to make it work with the enhanced code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@behnam-zakeri I've just pushed a couple commits with the requested changes.

Per this:

users that they need to re-parameterize some existing sets and parameters in their scenario to make it work with the enhanced code.

Did you already write some documentation or helper code about how they should do this? I think we ought to provide some…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed another couple of commits to add a helper function expand_dims().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, @khaeru both for expanding the doc, the elegant workaround for adding the re-initialized items, and the help function. It looks like very well-structured code and fits the purpose nicely.

behnam-zakeri added a commit to behnam-zakeri/message_ix that referenced this pull request Oct 5, 2022
@behnam-zakeri behnam-zakeri marked this pull request as ready for review October 5, 2022 09:01
@behnam-zakeri
Copy link
Contributor Author

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
Copy link
Contributor Author

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

khaeru added a commit to behnam-zakeri/message_ix that referenced this pull request Oct 19, 2022
@adrivinca
Copy link
Contributor

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.

@behnam-zakeri
Copy link
Contributor Author

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

@@ -51,8 +51,8 @@
* :math:`REL_{r,n,y} \in \mathbb{R}` Auxiliary variable for left-hand side of relations (linear constraints)
* :math:`COMMODITY\_USE_{n,c,l,y} \in \mathbb{R}` Auxiliary variable for amount of commodity used at specific level
* :math:`COMMODITY\_BALANCE_{n,c,l,y,h} \in \mathbb{R}` Auxiliary variable for right-hand side of :ref:`commodity_balance`
* :math:`STORAGE_{n,t,l,c,y,h} \in \mathbb{R}` State of charge or content of storage at each sub-annual timestep
* :math:`STORAGE\_CHARGE_{n,t,l,c,y,h} \in \mathbb{R}` Charging of storage in each sub-annual timestep (negative for discharging)
* :math:`STORAGE_{n,t,l,c,y,h} \in \mathbb{R}` State of charge or content of storage at each sub-annual time slice
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't you add the mode set here as well? or is it for version compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for noting this. I went through and added some missing indexes in the inline documentation.

behnam-zakeri added a commit to behnam-zakeri/message_ix that referenced this pull request Nov 8, 2022
@behnam-zakeri
Copy link
Contributor Author

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

@khaeru
Copy link
Member

khaeru commented Nov 21, 2022

@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 pushed a commit to behnam-zakeri/message_ix that referenced this pull request Nov 21, 2022
khaeru added a commit to behnam-zakeri/message_ix that referenced this pull request Nov 21, 2022
Copy link
Member

@khaeru khaeru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to merge once checks all pass.

@behnam-zakeri
Copy link
Contributor Author

Ready to merge once checks all pass.

Thanks a lot @khaeru for finalizing this.

@khaeru khaeru added this to the 3.7 milestone Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enh New features & functionality timeslice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storage mapping sets do not consider mode of operation of technologies
3 participants