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

Use times-info file for parameter mappings #148

Merged
merged 13 commits into from
Dec 15, 2023
Merged

Conversation

siddharth-krishna
Copy link
Collaborator

@siddharth-krishna siddharth-krishna commented Dec 6, 2023

As discussed, I'm getting mappings for parameters from the times-info.json config file. The rule I'm using is that parameters starting with UC are extracted from the ~UC_T table, and all others from ~FI_T.

There are some issues still:

  1. The parameters B and E are in times-info.json, but they need to be extracted from ~TimePeriods. I'd rather not modify the rule above to have a special case for these two, is it worth considering adding the tag to times-info.json?
  2. The code comparing the output to the DD/CSV ground truth now issues warnings that the headers in the CSV file (generated from times_mapping.txt) don't match the headers we generate (from the new mappings). I suppose when this isn't a big issue for now and when we deprecate times_mapping.txt we can switch to generating CSVs using the new mappings as well? For now, I've changed the compare code to still compare even if col headers don't match.
  3. The regression in Demo 5 is in ~UC_COMNET, for some reason the commodity isn't being exploded..
  4. There are some hacks in the PR code, because times-info.json uses different field names than we do. I propose we change them in the json file to be consistent with the code?

@olejandro
Copy link
Member

1. The parameters `B` and `E` are in `times-info.json`, but they need to be extracted from `~TimePeriods`. I'd rather not modify the rule above to have a special case for these two, is it worth considering adding the tag to `times-info.json`?

I have not been through the changes yet, but how about:

  • removing all the entries relying on UC_T or FI_T from times_mapping.txt
  • processing times-info.json after times_mapping.txt and only for attributes found either in UC_T or FI_T just before producing merged_tables.txt

@olejandro
Copy link
Member

@siddharth-krishna I can see that my previous suggestion is not relevant. :-)
It seems that the regression is caused by the normalisation in the mapping (btw, it should not be necessary) which resulted in wrong columns being used. Actually those columns stop being useful at certain stage, so I've dropped them. As a result, the merged file is cleaner and I hope it also makes it easier to see why the normalisation is not necessary.

@Antti-L
Copy link

Antti-L commented Dec 14, 2023

The rule I'm using is that parameters starting with UC are extracted from the ~UC_T table, and all others from ~FI_T

Note that parameters starting with UC can be defined also in any TFM_* tables. The Column for specifying the UC_N in TFM_* tables is UC_N. As always, the scenario order in the run case will determine, which value will survive (when the same instance of a parameter is defined in several scenarios), But I am not sure what will happen if the same parameter is defined in several tables in the same scenario (e.g. both ~UCT and TFM_* in the same scenario), haven't tested that.

@olejandro
Copy link
Member

Thanks @Antti-L. That's a very good point. Do you know whether it is used in any of the Demos (i.e. to know whether we have a test case available already)?

Regarding input data overwriting when it is specified in the same workbook, we don't explicitely control it yet. Now it depends on the order the tables are read in and since we do use parallel processing, it can vary. I believe it is a good idea to implement a rule to ensure consistent result in case there is overwriting.

@Antti-L
Copy link

Antti-L commented Dec 14, 2023

Do you know whether it is used in any of the Demos (i.e. to know whether we have a test case available already)?

I cannot be sure, but I doubt that not in the Demos. However, I am, for one, using such quite a lot for scenario-specific UC data.

@olejandro
Copy link
Member

@siddharth-krishna I've removed the part of the code that was doing renaming in the mapping; please check.

@olejandro
Copy link
Member

The remaining regression on Ireland is due to the tool currently not filtering out non-valid combinations of processes and commodities specified in UC_T, e.g. for UC_FLO.

@olejandro olejandro marked this pull request as ready for review December 15, 2023 04:03
@olejandro olejandro merged commit 5a6e2cb into main Dec 15, 2023
1 check failed
@olejandro olejandro deleted the sidk/new-mappings branch December 15, 2023 17:36
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