Expand the usage of TimesModel class #165
Conversation
|
@siddharth-krishna please feel free to edit this directly |
utils/dd_to_csv.py
Outdated
| with open("xl2times/config/times-info.json") as f: | ||
| attributes = f.read() | ||
|
|
||
| attributes = eval(attributes) |
There was a problem hiding this comment.
In general this is a potential security risk: reading arbitrary python code from a file and executing it! It is safer to use json.load to read from a JSON file.
There was a problem hiding this comment.
Well, since the contents of the file is controlled by us, I thought it was not that much of a risk. :-)
The main advantage of eval over json.load for loading json files that I see is that it seems to be able to interpret the nested structure correctly. json.load stops nesting at some point and just interpretes the remaining nested structure as a string.
I agree that json.load works perfectly fine here. :-)
| headers_data = dict() | ||
| headers_data = headers_by_attr | ||
| # The following will overwrite data obtained from headers_by_attr | ||
| # TODO: Remove once migration is done? |
There was a problem hiding this comment.
Should we change this code to use the header data from times-info.json instead?
There was a problem hiding this comment.
Do you mean exclusively? Currently times-info.json does allow specifying all the info that is included in times_mapping.txt. That's why I kept it for now.
There was a problem hiding this comment.
Ah I read the code hastily yesterday and thought it was overwriting everything in headers_by_attr. I now see that it only overwrites the few mappings that remain in the times_mapping.txt.
| "UserConstraints": model.user_constraints, | ||
| "Units": model.units, | ||
| }.items(): | ||
| tables[k] = v |
There was a problem hiding this comment.
Is this intended to be a temporary solution -- that we copy the tables back from the TimesModel instance to the Dict[str, DataFrame] so that the produce_times_tables can work as before? I'm wondering if we'd eventually want to move to encapsulating all the model data in TimesModel and having a TimesModel.produce_tables function that produces the output.
There was a problem hiding this comment.
Yes, that's right. Switching to e.g. TimesModel.to_gams() later on would be great!
|
Also, CI is failing because esma-cu/TIM#4 removed the file |
No description provided.