-
Notifications
You must be signed in to change notification settings - Fork 129
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
extend industry/subsectors to explicitly track FE by SE origin #1620
extend industry/subsectors to explicitly track FE by SE origin #1620
Conversation
62a2115
to
120f5c7
Compare
vm_demFENonEnergySector(t,regi,entySe2,entyFe,"indst",emiMkt) | ||
) | ||
=e= | ||
q37_feedstocksLimit(t,regi,entySe,entyFe,in,secInd37,emiMkt)$( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this restriction with the new formulation of q37_demFeFeedstockChemIndst
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which one? The rats-nest of lines 244-252? Actually, yes. This can be cleaned up by introducing some specialised mappings for the purpose, but the beauty treatment is scheduled for next week ;)
awesome! thank you for this, I think it will make things more tractable |
@JakobBD PBS is widely infeasible with this ( |
What does that mean for the merging process? As it's a new feature, it may not have to be merged to develop right away? Do you think you could make the PR a WIP, do your runs in your own branch and wait with merging until the pbs part is sorted out? |
Well, it is a new feature, but one that (hopefully) solves two problems – feedstock composition and solver performance. |
AND secInd37_2_pf(secInd37,in) | ||
AND secInd37_emiMkt(secInd37,emiMkt) | ||
AND entyFe2Sector(entyFe,"indst") | ||
AND entyFeCC37(entyFe) ) .. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entyFeCC37
one might be redundant. I did not want so spend a test run on finding out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is ppfen_industry_dyn37(in)
, made redundant by in_chemicals_feedstock_37(in)
, I suppose. But I wasn't even referring that, just looking in awe at 9 conditions...
AND secInd37_emiMkt(secInd37,emiMkt) ) .. | ||
sum((sefe(entySe,entyFe), | ||
fe2ppfEn(entyFe,in)), | ||
v37_demFeIndst(t,regi,entySe,entyFe,in,emiMkt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually quite a fundamental problem: ppfen
do not exist any more for process-based sectors, so the whole variable definition (and thus the FE balance) is invalid for them.
Could you maybe define v37_demFeIndst(t,regi,entySe,entyFe,secInd37,emiMkt)
instead? That would still give you the full information, except for the information of primary vs secondary steel. Otherwise, you'd have to skip that intermediate step for pbs and add its share directly to q37_demFeIndst
- would be a shame to not have that tracking for pbs, and I don't know what implications that would have for reporting.
Sorry about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bottom line: PR needs significant changes in order to not break the process-based implementation. @LaviniaBaumstark
Nice stuff otherwise! Sorry about the pbs hassle! |
many thanks for trying to improve teh situation! I agree that this is not ready for merging. Let's try to solve it on Monday and I will start calibration runs afterwards and we see how far we get for the validation. Maybe we will try to find a new date for a validation meeting before easter. |
Michaja's test runs seem to converge |
fc6bfd6
to
ab49a9a
Compare
Test runs here
|
Cleaning of conditions and sets fill be a follow-up. |
ab49a9a
to
ca1752a
Compare
fe2mat(all_enty,all_enty,all_te) "" / / | ||
secInd37_tePrc(secInd37,tePrc) "" / / | ||
fe2ppfen_no_ces_use(all_enty,all_in) "" / / | ||
$endif.cm_subsec_model_steel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is that needed for? Seems hard to maintain (whoever adds a set needs to know/remember there's this list lurking down here that it needs to be added to as well...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, GAMS will help with a friendly compilation error in case something needs to be added here. :)
Only the sets used in set calculations down below, or somewhere else in the code, are required to be here. I just did not want to figure out one-by-one which those are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you got rid of all the conditions above! Please revert that! Two reasons:
- The one on maintainability I posted above
- There will be other process-based subsectors soon. The sets are for all subsectors, the entries are subsector-specific. Your implementation will not allow switching implementations for sectors individually.
*** be removed once the variable is established. | ||
loop ((t,regi, | ||
sefe(entySe,entyFe), | ||
fe2ppfEn(entyFe,in), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may need adaptation to ue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory yes, but testOneRegi
was not screaming bloody murder, so probably it is irrelevant now.
Looks good, thanks for the work, let's see what the calibration says. |
2d8190f
to
7a6a0ca
Compare
- limit the changes in prices for selected production factors to between 0.5-2 of the price of the previous calibration iteration - this might stabilise calibration behaviour against sudden price jumps
…ead of ppfen to be compatible wit process-based implemen-based implementation
7a6a0ca
to
8b8fb43
Compare
7b304ae
to
25e9f6d
Compare
Status update
I would merge this now, and tinker with calibration and Nash performance based on the |
I agree with merging and further working on improving the develop branch. Could you please also increase the input data version to 6.74 |
75039ce
into
remindmodel:develop
I cannot. Line 30 in 54c883c
|
eeh, 6.75, but will do it later |
It does not solve the issue of slow convergence, but on the PBS run, maybe you could try to run the calibration relaxing the tax convergence criteria at first. |
PS: it would be nice to the person that is developing the PBS version to check also the gdx from the iteration that got infeasible running some testoneregi runs. |
I do not think there is much information to be gained. In all likelihood, CONOPT just ran into some NA. |
As expected, cannot replicate. |
This extends
industry/subsectors
to explicitly track the SE-origin on FE demand. So far, we assumed identical SE-origin shares in FE demand across industry subsectors in order to report them (REMIND itself did not much care), but this collided with the assumptions made about chemical feedstocks (see #1561, #1576).Before, we equated total industry FE supply (summed over SE origins) with industry FE demand (summed over subsectors). Now we introduce a new variable,
v37_demFeIndst(t,regi,entySe,entyFe,in,emiMkt)
, for industry FE demand that is explicit in FE carrier, SE origin, and industry subsector (and emissions market). This allows foro37_demFeIndSub
to report SE-specific subsector FE demand, without assuming SE-shares in FE carriers.Two calibrations with these changes (
SSP2EU-NPi
andSSP2EU-EU21-NPi
) are running under/p/tmp/pehl/Remind/
(_fast
variants on the priority queue, others on medium).Checklist:
FAIL 0
in the output ofmake test
)CHANGELOG.md
has been updated correctly