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

Fill other_indexes in more a general way #155

Merged
merged 12 commits into from
Mar 11, 2024
Merged

Fill other_indexes in more a general way #155

merged 12 commits into from
Mar 11, 2024

Conversation

olejandro
Copy link
Member

@olejandro olejandro commented Dec 18, 2023

This PR uses information specified veda-attr-defaults.json to fill other_indexes.

@Antti-L
Copy link

Antti-L commented Dec 18, 2023

I just had a glance of what is being done here, and I have the following comment:
One of the main ideas of Comm-In-A / Comm-Out-A is that they are auxiliary flows that should never participate in efficiency (ACT_EFF) equations. And that is enforced by VEDA automatically defining a zero ACT_EFF for any auxiliary flows. A zero efficiency will make the flow disappear from the efficiency group.

Now, what I am seeing in the diffs is e.g. commodity-in-aux being added for CEFF-I and CEFFICIENCY, which I thus find a bit confusing, because auxiliary flows are supposed to be, by definition, excluded from efficiency groups (i.e. ACT_EFF groups).

[Edit:] Ok, I tested it under VEDA and conclude that a user-defined value would override the zero (which is only defined for the default year, usually the Base year), and so including the auxiliary commodities might still be ok, although somewhat error-prone. Nonetheless, I would myself consider that it might be better to allow overriding it only with the full-blown ACT_EFF.

@Antti-L
Copy link

Antti-L commented Dec 18, 2023

Ahh... I also see auxiliary flows being added to SHARE, SHARE-I and SHARE-O. This looks also very strange, because SHARE, SHARE-I and SHARE-O operate on the VEDA-defined groups e.g. NRGI, NRGO etc., from which the auxiliary flows have been specifically removed (unlike the TIMES attribute FLO_SHAR, which operates on any user-specified CG). Therefore, again I am confused by the code change, but could be misinterpreting it.

[Edit:] I tested it under VEDA, and the test seemed to confirm that defining a SHARE, SHARE-I and SHARE-O does not work for auxiliary commodities. So, I would indeed suggest not to include those auxiliary flows for them. But for the TIMES FLO_SHAR it is ok.

@olejandro
Copy link
Member Author

@Antti-L thanks a lot for your input on this and apologies for lack of interaction. I'll be resuming the work on this PR now.

@olejandro
Copy link
Member Author

@Antti-L, quick question, in case you happen to know. The figure below is from DemoS 9. The value specified by Output never makes it to the dd files. Is it because DIIS is primary CG?

image

@Antti-L
Copy link

Antti-L commented Mar 8, 2024

Yes, right. VDA_FLOP would be in most cases be meaningless for a PG commodity. There is only a special case where it can be used for such (which I think would work in scenario files).

And, as far as I remember, VEDA would even complain about Output for a PCG, if it is not exactly 1 (the value 1 is a shortcut for a topology entry, and many models use that method, i.e. no topology is explicitly specified, only Output).

@olejandro
Copy link
Member Author

Thanks @Antti-L! So should we also discard VDA_FLOP specified for PCG in xl2times? When should we keep it?

@Antti-L
Copy link

Antti-L commented Mar 8, 2024

Yes, I think we should discard VDA_FLOP for PCG commodities, at least in FI_T tables. If it is possible to allow it in TFM tables, that would allow using it for the special functionality (which is kind of a relic from an older design). It can be used as multipliers for another VDA_FLOP specified for a group on the shadow side, which offers a "unique" functionality when the PCG has several commodities (such functionality is not available otherwise, apart from user constraints).

@olejandro
Copy link
Member Author

It can be used as multipliers for another VDA_FLOP specified for a group on the shadow side, which offers a "unique" functionality when the PCG has several commodities (such functionality is not available otherwise, apart from user constraints).

Interesting! I would definitely need some more details to understand how to implement this. A test case would be ideal. Is this "unique" functionality part of TIMES or Veda?

@Antti-L
Copy link

Antti-L commented Mar 8, 2024

Interesting! I would definitely need some more details to understand how to implement this. [...] Is this "unique" functionality part of TIMES or Veda?

Ehh.., don't quite follow: I just said what would be needed: Allow specifying VDA_FLOP for PCG commodities in TFM tables. If you can do that, then it would be a functionality part of Xl2times. I have not actually tested whether VEDA2 does allow that (but I have thought it does, and if so, it would be a functionality part of VEDA2). Implementation of all model equations are part of TIMES, so I guess it should be clear that the model equations are, in a way, a functionality of TIMES?

@olejandro
Copy link
Member Author

Well, what I think would be reasonable to do is to drop VEDA_FLOP data entries specified for PCG, before exporting data to GAMS as a kind of quality assurance. What I don't understand is why to distinguish between the entries coming from FI_T and TFM? That is why I am asking where that "unique" functionality is found.

@Antti-L
Copy link

Antti-L commented Mar 8, 2024

Ok, so it seems you are saying that VEDA2 does not distinguish between entries coming from FI_T and TFM? As I said, I was in the belief that it does. I am sorry if I was wrong. As to possible reasoning for such distinguishing, if such would exist, I think the implicit topology feature in FI_T (which I mentioned above) is indeed one understandable reason. You cannot define topology by using Input / Output (aka VDA_FLOP) in TFM tables, but you can in FI_T tables.

@olejandro
Copy link
Member Author

Ok, so I guess, the short answer is that the "unique" functionality is part of Veda. Please bare in mind that we are not trying to reimplement Veda and so the internal processing steps may not be the same. Some of the functionality may be implemented quite differently or not supported at all. Ultimately whether or not a particular feature is supported will depend on the needs of the community and the resources available.

@Antti-L
Copy link

Antti-L commented Mar 9, 2024

I think I did not even request anything about supporting VDA_FLOP for PCGs (although I have understood that xl2times should be able to reproduce VEDA models). It was you who asked my opinion about discarding them, and I only answered positively. And then, I also mentioned that "If it is possible to allow it in TFM tables, that would allow using it for the special functionality (which is kind of a relic from an older design)." So, can you explain why you now say "Please bare in mind that we are not trying to reimplement Veda and so the internal processing steps may not be the same. "? Did I somewhere suggest reimplementing VEDA?

But sure, if VEDA allows VDA_FLOP in TFM tables, that is a functionality in VEDA (i.e. supporting VDA_FLOP is a functionality of VEDA). But there is nothing more about it in VEDA, because the implementation of model equations is inside TIMES.

@olejandro
Copy link
Member Author

@Antti-L , you write:

it seems you are saying that VEDA2 does not distinguish between entries coming from FI_T and TFM?

Which, since I never said that, seems to imply that xl2times will do exactly how Veda does? Or what did you mean?

Please don't get me wrong, I really appreciate your advise and feedback. At the same time I still don't understand what difference should it make whether one uses FI_T of TFM to define VDA_FLOP on PCG for whether the data entry makes it to the dd file or not. If defining VDA_FLOP on PCG does not make sense from TIMES perspective then, I believe, the data entry should be dropped and the user should be informed about it.

Currently xl2times does not check whether there is any VDA_FLOP specified on a PCG. I am planning to add such a check just before export to dd files. This implies that e.g. any TFM_MIG tables that generate data based on it should still work.

@Antti-L
Copy link

Antti-L commented Mar 9, 2024

I already explained that VDA_FLOP specified for PCG commodities can be used for a special functionality in TIMES. The functionality is related to model equations, and is not related to VEDA at all. But because VEDA distinguishes between the entries coming from FI_T and TFM, you cannot specify such in FI_T tables by using Input or Output. Input / Output with a value of 1 implicitly define topology entries in FI_T tables, and other values defined for PCG commodities just trigger errors in VEDA. But TFM tables work differently: One cannot define topology entries by Input / Output in TFM tables (I think that is good so), and VDA_FLOP (hopefully) works there also for PCG commodities.

As I have tried to explain, VDA_FLOP for PCG commodities makes perfect sense in TIMES, from the TIMES perspective.

@olejandro
Copy link
Member Author

Thanks a lot @Antti-L. It is much clearer and makes perfect sense now. :-)

@olejandro olejandro changed the title Add additional aliases to veda-attr-defaults.json Fill other_indexes in more a general way Mar 10, 2024
@olejandro olejandro marked this pull request as ready for review March 10, 2024 16:10
@olejandro
Copy link
Member Author

@siddharth-krishna this PR introduces a minor regression (+8 rows in total on Demos 9-12) due to treatment of INPUT and OUTPUT (Veda aliases of VDA_FLOP). Based on my discussion with @Antti-L , I believe it is okay to have those extra rows and we don't need to do anything about them (yet).

@olejandro olejandro merged commit 6e23fb4 into main Mar 11, 2024
1 check failed
@olejandro olejandro deleted the olex/vda_flop branch March 11, 2024 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants