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

OK to remove sefe and fete? #931

Open
0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q opened this issue Aug 3, 2022 · 11 comments
Open

OK to remove sefe and fete? #931

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q opened this issue Aug 3, 2022 · 11 comments
Assignees
Labels
code cleaning Code that could/should be cleaned up question Further information is requested

Comments

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

You added these two sets a while back

remind/core/sets.gms

Lines 2504 to 2505 in 043449b

sefe(all_enty,all_enty) "map secondary energy to final energy"
fete(all_enty,all_te) "map final energy to technologies"

but they are used nowhere. Also they don't comply to the naming convention (no sacred "2" in there) and is there a case where se2fe can't be used with either entySE or te ignored?
OK to remove them?

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q 0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q added question Further information is requested code cleaning Code that could/should be cleaned up labels Aug 3, 2022
@Renato-Rodrigues
Copy link
Member

I use them in ECEMF code that will be soon merged to REMIND, and ARIADNE uses them also in other code that should be also merged to REMIND.
I know the naming convention does not follow the "a2b" standard, but the alternative in my opinion would be to rename the current se2fe to se2fe_te (or se2fete), and use se2fe in this set instead, as the te dimension in this set group is useless anyway as fe and te is a one to one correspondence right now in the model.

I just didn't do that yet as this was a much wider change to the model and R libraries than simply adding a new set.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

But you can still use se2fe instead of sefe and just ignore the te dimension …

@Renato-Rodrigues
Copy link
Member

Renato-Rodrigues commented Aug 3, 2022

I could, but this would force me to control the te set, even if it does not need to be included in a equation or variable.
So I would need to either include a dummy sum (sum(te$se2fe(entySe,entyFe,te), .... )), or unnecessarily increase the dimension of a variable or equation with te.
Either the code gets more complicated then needed, or the dimension of the variables/parameters include a useless dimension which are highly undesirable in my opinion..

@Renato-Rodrigues
Copy link
Member

Renato-Rodrigues commented Aug 3, 2022

For example, check the FE balance equations in the demand modules.
Although te is never carrier after prodFe, i.e. vm_demFeSector does not include the te dimension and neither any variable that comes after it, if I want to select only the acceptable combinations of entySe and entyFe I need to control te in the balance equation with a dummy te sum:

q36_demFeBuild(ttot,regi,entyFe,emiMkt)$((ttot.val ge cm_startyear) AND (entyFe2Sector(entyFe,"build"))) .. 
  sum((entySe,te)$se2fe(entySe,entyFe,te), vm_demFeSector(ttot,regi,entySe,entyFe,"build",emiMkt)) 
  =e=
...
;

Referring to te in this equation is completely unnecessary, just complicating code understanding for somebody that is not familiar with it. Besides, this is even "uglier" in the case the sum is only there to get rid of the te dimension, as it is the case in some of the code that will be merged to the trunk soon.

For parameters and variable bounds this is even worst, as you need to add an extra loop over te complicating more than necessarily the code.
instead of writing:

par(entySe,entyFe)$sefe(entySe,entyFe) = 1;

you need to write:

loop((entySe,entyFe,te)$se2fe(entySe,entyFe,te),
   par(entySe,entyFe) = 1;
);

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

Referring to te in this equation is completely unnecessary, just complicating code understanding for somebody that is not familiar with it. Besides, this is even "uglier" in the case the sum is only there to get rid of the te dimension, as it is the case in some of the code that will be merged to the trunk soon.

I disagree. I think having just one set linking entySE to entyFE that includes te which may or may not be ignored is clearer than having two separate sets with basically identical names. While the te dimension in se2fe strictly speaking is redundant, there is a symmetry to the pe2se and se2se sets. Which is why variables like vm_prodFE include this "superfluous" dimension. (I don't think it is ever going to happen, but it would be conceivable that we could have different T&D technologies for the same entySE/entyFE set, in which case the te dimension suddenly would become crucial.)

For parameters and variable bounds this is even worst, as you need to add an extra loop over te complicating more than necessarily the code.

Because you defined them one dimension short.

par(se2fe(entySE,entyFE,te)) = 1;

works just fine (at no extra cost of memory).

I would suggest we ask Lavinia to make a ruling on this. In my view it is a deviation from established REMIND coding practice. We should be positive it is not something we do not want, before lots of code gets merged. Is there already remind2 code for these equations/variables/parameters that would be affected if we decide to change this?


As a general note, you can use mappings directly as index lists, without the dollar notation, shortening code and improving readability.

q36_demFeBuild(ttot,regi,entyFe,emiMkt)$((ttot.val ge cm_startyear) AND (entyFe2Sector(entyFe,"build"))) .. 
  sum(se2fe(entySe,entyFe,te), vm_demFeSector(ttot,regi,entySe,entyFe,"build",emiMkt)) 
  =e=

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented Aug 4, 2022

@LaviniaBaumstark, see discussion above.

TL;DR

  • se2fe is a one-to-one-to-one mapping of entySE, entyFE, and te, which makes the te dimension (or any one other dimension) superfluous.
  • Renato added a new set sefe without the te dimension to shorten code and improve readability.
  • I took offence on the name of the set, which isn't used in develop, but Renato informed me that it will be once ECEMF and ARIADNE code is merged.
  • My argument is that the symmetry between pe2se, se2se, and se2fe is worth having the "extra" te dimension around, even if that means defining equations/variables/parameters over one extra set (which does not cost any memory).
  • We should decide on this before merging any code.

@Renato-Rodrigues
Copy link
Member

If you want to follow a consistent naming convention you need to firstly rename the se2fe(entySe,entyFe,te) set to include the te reference in its name, even more if you consider the argument that in the future we could lose the one to one relationship between entyFE and te, which I agree is indeed a real possibility. Following the same logic, se2fe should be renamed to se2fe2te and the current sefe would be the actual se2fe, as I mentioned in the first message I posted in the issue.

@Renato-Rodrigues
Copy link
Member

By the way, including extra unnecessary dimensions to any parameter or variable, even if it is a one to one relationship, increases memory usage (not substantially, but it does), code length, reduces readability and requires additional filtering in R reporting code.

@Renato-Rodrigues
Copy link
Member

As a general note, you can use mappings directly as index lists, without the dollar notation, shortening code and improving readability.

Yes, I know. But from a code reading perspective it is usually clearer to instate which dimensions are actually controlled by the sum, versus which ones are controlled by some other part of the equation.

@Renato-Rodrigues
Copy link
Member

Because you defined them one dimension short.

I disagree. I am not defining one dimension short my parameter, because in my case there is no possible relationship between the te dimension and the parameter I am calculating. Even if you drop the one to one relationship, the parameter will be always independent of te in final energy level in my case, so including a dummy dimension serves no goal in cases like calculating final energy prices or sector specific markups per carrier in each sector for example.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

  • I see your point on the controlling of sets, although I disagree. De gustibus non est disputandum.
  • Mincing kilobytes for the .gdx file is rather petty.
  • I never said the naming convention is consistent, just that it happens to be there. ;)

I'm not much here or there in this issue, just stumbled on the sets while reviewing Simon's code. If it (the ECEMF and ARIADNE code) gets merged, then it should get merged as a positive decision to do things this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleaning Code that could/should be cleaned up question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants