Create trade links #167

olejandro · 2024-01-26T04:21:15Z

No description provided.

olejandro · 2024-02-08T04:38:58Z

@siddharth-krishna, any chance you could take a look at the changed added transforms?

siddharth-krishna · 2024-02-08T05:11:43Z

xl2times/transforms.py

+                raise ValueError(f"TFM_INS-TS table already has Year column: {table}")
+            # TODO: can we remove this hacky shortcut? Or should it be also applied to the AT variant?
+            if set(df.columns) & query_columns == {"cset_cn"} and has_no_wildcards(
+                df["cset_cn"]


@olejandro do you understand why we have this condition? Unless there's a compelling performance reason, it's probably better to have one general way of handling all TFM_INS-TS tables.

Let's handle this in a subsequent PR.

siddharth-krishna · 2024-02-08T05:25:40Z

xl2times/transforms.py

+    for table in tables:
+        if table.tag == datatypes.Tag.tradelinks:
+            df = table.dataframe
+            comm = df.columns[0]


Are we sure that the first column is always what we want? The danger of this kind of integer indexing is silent failures if someone adds another transform in the middle that changes column orders. Perhaps better to fetch column by name, which will raise an understandable error if that column isn't present.

In this case, yes. We have to rely on the position to retrieve the column name. We could add a check later that the name is on the commodities list.

siddharth-krishna · 2024-02-08T05:45:30Z

xl2times/transforms.py

+            df = table.dataframe
+            comm = df.columns[0]
+            destinations = list(df.columns).remove(comm)
+            df.rename(columns={comm: "origin"}, inplace=True)


I think we should try to avoid using the inplace keyword whenever possible (see this proposal for more details).

Also, the commodity is renamed to origin here, but later in the code origin becomes reg1 which sounds like a region? I'm confused.

I guess we should address the inplace issue systematically then? Maybe through a subsequent PR?

For the renaming issue, it is due to the table format. What we have is like this:

elc reg1 reg 2

REG1 1

REG2 1

And what we want to get is this:

origin destination comm1 comm 2 comm process tradelink

REG1 REG2 ELC ELC ELC TB_ELC_REG1_REG2_01 b

@olejandro : Do you know what is the purpose of the third comm? I have always wondered about that...

Ah I see! I misread the comm = df.columns[0] as though comm was the entire column, not just the column label. So this essentially turns an adjacency matrix for one type of edge into an adjacency list with edge labels.

@olejandro : Do you know what is the purpose of the third comm? I have always wondered about that...

No... Tbh, I was planning to ask you. :-)

xl2times/transforms.py

siddharth-krishna · 2024-02-08T05:54:47Z

xl2times/transforms.py

+    model: datatypes.TimesModel,
+) -> List[datatypes.EmbeddedXlTable]:
+    """
+    Transform tradelinks table


Suggested change

Transform tradelinks table

Transform tradelinks table to tradelinks_dins

siddharth-krishna · 2024-02-08T06:01:25Z

xl2times/transforms.py

+            df.drop(columns=["tradelink"], inplace=True)
+            df = df.merge(td_type, how="inner", on="regions")
+            # Drop tradelink (bidirectional) duplicates
+            df.drop_duplicates(subset=["regions"], keep="last", inplace=True)


It's funny that we drop bidirectional links here but reconstruct them in generate_trade :)

Yes, that's because we want to get rid of the same bidirectional tradelinks that have different process names. Otherwise we would get 4 tradelinks in generate_trade instead of 2. :-)

That's only if you put a b in the tradelinks column. :)

I was wondering if it's possible to simplify the code and leave both directions of links here, if it allows removing the code in generate_trade that "doubles" the rows with a b?

Well, if we don't put b in the column, the links will be processed as unidirectional. :-) This may have an impact on how some other attributes / constraints are applied.

I was wondering if it's possible to simplify the code and leave both directions of links here, if it allows removing the code in generate_trade that "doubles" the rows with a b?

Not that I see...

siddharth-krishna · 2024-02-08T06:04:00Z

xl2times/transforms.py

+            td_type = df.groupby(["regions"])["tradelink"].agg("count").reset_index()
+            td_type.replace({"tradelink": {1: "u", 2: "b"}}, inplace=True)
+            df.drop(columns=["tradelink"], inplace=True)
+            df = df.merge(td_type, how="inner", on="regions")


This code block above looks good but I do wonder if this sort of thing (melt + count links + create new DF) would actually be faster in plain Python -- it might also be more understandable! It's okay for now I think but I'd be curious to test it sometime. :)

I believe we have other instances of code blocks like this. Maybe worthwhile investigating!

Co-authored-by: Siddharth Krishna <[email protected]>

olejandro · 2024-02-08T14:37:54Z

@Antti-L, any chance you could share your opinion on this PR? Namely, currently, the changes don't take into account the sheetname a tradelinks table is found on. This leads to e.g. two unidirectional tradelinks specified in opposite directions in uni sheet to be interpreted as one bidirectional tradelink. Would you find it important to keep those tradelinks unidirectional?

Antti-L · 2024-02-08T14:56:51Z

Would you find it important to keep those tradelinks unidirectional?

I think it is important if one wants that with xl2times one should be able to reproduce a model that has been built under VEDA. I think that was indeed originally the idea, even though options with more flexibility / better functionality could be also supported by xl2times. But if you would treat two unidirectional links as a single bi-directional link, the model would be different, and any process parameters defined for either would become erroneous, no? The model solution would most likely be different and possibly notably incorrect.

BTW: In trade link matrices one should also be able to specify the name of the process in each cell having a link. I am using that heavily in my models. Is that supported already by Xl2times?

Antti-L · 2024-02-08T15:37:47Z

I just happened to see this statement in the code (part "Do not create trade links for not included regions"):
filter_regions = model.internal_regions.union({"IMPEXP", "MINRNW"})
If this prevents creating trade links with any other external regions, that is incorrect. Only links connecting one external region to another external region may well be excluded.

olejandro · 2024-02-08T15:46:25Z

I just happened to see this statement in the code (part "Do not create trade links for not included regions"):
filter_regions = model.internal_regions.union({"IMPEXP", "MINRNW"})
If this prevents creating trade links with any other external regions, that is incorrect. Only links connecting one external region to another external region may well be excluded.

Thanks @Antti-L! This does not yet take into account #162. Only when regions are specifically filtered (e.g. similar to specifying regions to include in Veda case manager).

olejandro · 2024-02-08T15:52:17Z

BTW: In trade link matrices one should also be able to specify the name of the process in each cell having a link. I am using that heavily in my models. Is that supported already by Xl2times?

Not yet, but it definitely could be. I would probably open a separate issue to clarify all the details though.

olejandro · 2024-02-08T15:55:17Z

I think it is important if one wants that with xl2times one should be able to reproduce a model that has been built under VEDA. I think that was indeed originally the idea, even though options with more flexibility / better functionality could be also supported by xl2times. But if you would treat two unidirectional links as a single bi-directional link, the model would be different, and any process parameters defined for either would become erroneous, no? The model solution would most likely be different and possibly notably incorrect.

Among the benchmarks available to us, it is only an issue for TIMES-NZ, but you are right...

…el-reader into create-trade-links

olejandro · 2024-02-09T04:04:35Z

This doesn't yet allow generating PRC_ACTUNT and PRC_TSL for trade processes created based on tradelinks.
@Antti-L, I would appreciate another advice from you here:

should PRC_TSL default to daynite if commodity of type ELC is in TOP_IRE for the process? Should it remain blank otherwise?
for CG in PRC_ACTUNT, should it be determined based on DEM, MAT, NRG, ENV, FIN order?
for unit in PRC_ACTUNT, should it be based on the commodity unit?

Antti-L · 2024-02-09T09:14:07Z

should PRC_TSL default to daynite if commodity of type ELC is in TOP_IRE for the process? Should it remain blank otherwise

I am not sure how it is defined in VEDA, if that's what you ask. I have always hoped for a FI_Process table for the trade processes as well, where I would be able to define the PCG and TSLVL, process subtype, units and vintaging. But no, KanOrs has thus far declined it. I suggest you test it under VEDA to see how VEDA works. Possibly the level is taken as the finest among the commodities traded?

for CG in PRC_ACTUNT, should it be determined based on DEM, MAT, NRG, ENV, FIN order?

Again, I am not sure how it is defined in VEDA, if there are several commodity types traded. But I think you could assume that they are all of the same type (I have never used mixed types in trade processes myself). So you could just take the first commodity type of the traded commodities, or if you like, use some priority order as deemed best or based on VEDA testing.

for unit in PRC_ACTUNT, should it be based on the commodity unit?

I guess that is indeed what VEDA does.

olejandro added 8 commits January 25, 2024 23:18

Add tradelinks_dins to veda-tags.json

12cc5cd

Merge branch 'main' into create-trade-links

530bf1e

Merge branch 'main' into create-trade-links

521dd31

Generate trade links between internal regions specified as dins

f0042ce

Do not create trade links for not included regions

12bb1a1

Transform tradelinks into dins type

e70fba0

Determine whether trade is bi- or unidirectional

db5ce7a

Make order of regions in the tuple deterministic

88aa3e7

siddharth-krishna reviewed Feb 8, 2024

View reviewed changes

Update xl2times/transforms.py

f47847e

Co-authored-by: Siddharth Krishna <[email protected]>

olejandro added 4 commits February 8, 2024 20:21

Use sheetname to determine the type of tradelink

9ba8c07

Merge branch 'create-trade-links' of github.com:etsap-TIMES/times-exc…

f07c641

…el-reader into create-trade-links

Merge branch 'main' into create-trade-links

c05eaec

Generate (missing) processes based on tradelinks

8e21a27

olejandro marked this pull request as ready for review February 9, 2024 03:47

olejandro requested a review from siddharth-krishna February 9, 2024 03:47

Merge branch 'main' into create-trade-links

83bb5db

siddharth-krishna approved these changes Feb 9, 2024

View reviewed changes

Add units, tslvl and primary cg to auto-generated IRE

ecaac02

olejandro merged commit 8d066d0 into main Feb 9, 2024
1 check passed

olejandro deleted the create-trade-links branch February 9, 2024 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create trade links #167

Create trade links #167

olejandro commented Jan 26, 2024

olejandro commented Feb 8, 2024

siddharth-krishna Feb 8, 2024

olejandro Feb 8, 2024

siddharth-krishna Feb 8, 2024

olejandro Feb 8, 2024

siddharth-krishna Feb 8, 2024

siddharth-krishna Feb 8, 2024

olejandro Feb 8, 2024

olejandro Feb 8, 2024

Antti-L Feb 8, 2024

siddharth-krishna Feb 8, 2024

olejandro Feb 8, 2024

siddharth-krishna Feb 8, 2024

siddharth-krishna Feb 8, 2024

olejandro Feb 8, 2024

siddharth-krishna Feb 8, 2024

olejandro Feb 8, 2024

olejandro Feb 8, 2024

siddharth-krishna Feb 8, 2024

olejandro Feb 8, 2024

olejandro commented Feb 8, 2024

Antti-L commented Feb 8, 2024

Antti-L commented Feb 8, 2024

olejandro commented Feb 8, 2024

olejandro commented Feb 8, 2024 •

edited

Loading

olejandro commented Feb 8, 2024

olejandro commented Feb 9, 2024

Antti-L commented Feb 9, 2024 •

edited

Loading

	Transform tradelinks table
	Transform tradelinks table to tradelinks_dins

Create trade links #167

Create trade links #167

Conversation

olejandro commented Jan 26, 2024

olejandro commented Feb 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olejandro commented Feb 8, 2024

Antti-L commented Feb 8, 2024

Antti-L commented Feb 8, 2024

olejandro commented Feb 8, 2024

olejandro commented Feb 8, 2024 • edited Loading

olejandro commented Feb 8, 2024

olejandro commented Feb 9, 2024

Antti-L commented Feb 9, 2024 • edited Loading

olejandro commented Feb 8, 2024 •

edited

Loading

Antti-L commented Feb 9, 2024 •

edited

Loading