Reduce TFM_INS-AT and TFM_INS-TS to TFM_INS

[siddharth-krishna]

Part 1 of #120

[Antti-L]

I looked at some of the files, and there I saw the following assumption being used:

A column name is a year if it is an int between 1001 and 4000

This is not sufficient for data columns, because any time series attribute can also be defined for the '0' Year index (which is not a real year, but the IE option placeholder, which must be supported as a Year data column header).

In addition, I wonder where the 1001 comes from; the year 1000 is supported by VEDA and TIMES as well. 😁

[siddharth-krishna]

Thanks @Antti-L . That code you saw was my first attempt at handling TFM_INS-TS, but I discovered that the code is already handling it (see my comments), so I removed that code. Looks like the existing code assumes all unknown columns are years -- is that sufficient?

[siddharth-krishna]

The code here assumes all columns that are not known_colums or {"region", "ts_filter"} are years

[siddharth-krishna]

Looks like we already have code to handle TFM_INS-TS

[olejandro]

Yes, we do. :-) The idea for #120 is to handle the variants before proceeding with other transforms. I.e. this way we would reduce the number of elif statetements and improve readability of the code.

[siddharth-krishna]

Ah yes, I was coming to the same conclusion myself! Let me see if I can refactor this code to make it simpler.

[Antti-L]

The code here assumes all columns that are not known_colums or {"region", "ts_filter"} are years

Ok fine with that basically, but I am not sure the known_columns include all known columns. For example, Top_check can be used to enforce a check that the (process,commodity) pair is indeed defined in the process topology (which is not done by default by VEDA, and so it probably should not be done here either). Is Top_check one of your known colmns for TFM_INS / TFM_INS-AT /TFM_INS-TS ? I did notice it under the UC_T transforms, but could not see it not under TFM_INS... Note that under UC_T, topology check IS done by default.

[olejandro]

Thanks @Antti-L. Probably not yet. This file includes an overview of columns names that will be supported.

[siddharth-krishna]

@olejandro do you know if this if condition and the elif directly below can be replaced by something more general? Perhaps this is something we also do to other TFM_INS variants?

[olejandro]

Basically, @siddharth-krishna I believe this part can just be dropped if we do reduction earlier.

[olejandro]

Btw, on generalising the code: we'll need to do this kind of reduction also for tfm_upd and tfm_dins

[siddharth-krishna]

Thanks for the review. I'll refactor this a bit so don't merge it yet.

[olejandro]

No problem. I thought I'd leave that decision up to you. :-)

[siddharth-krishna]

I fixed the regressions! This PR now reduces TFM_INS-TS and TFM_INS-AT tables to TFM_INS tables.

The little shortcut hack that converts some TFM_INS-TS tables to FI_T is still there because removing it leads to more regressions, so I've left removing it to a future PR.

The new code assumes all columns whose names are integers >= 0 are years in TFM_INS-TS tables. Let me know if that assumption is false.

[olejandro]

Thanks! Looks good.