Extend application of defaults beyond FI_T tables #240
Conversation
|
@siddharth-krishna any chance you could try this locally, e.g. on Ireland? I am surprised CI shows no difference. |
|
Oh good catch! Looks like there's something wrong with the CI... I'll investigate! |
|
#250 should make it easy to reduce the number of additional rows on Ireland. |
|
Using |
|
Great news, Olex, regarding DataModule 🚀 I pushed my quick fix for #246, which is to fix a precision of (There are 2 missing rows from COM_PROJ in Demos 9-12 but perhaps that's what you were talking about w.r.t. order of rows?) Do you have an idea of what a good precision is for outputting / comparing to ground truth? |
|
Yei!! 🚀
Not really... Maybe we could use a trial and error approach to set it as high as possible?
Could be! I'll check it out. |
|
@siddharth-krishna, somehow there seems to be a issue in the gdx comparison. Could you check? |
An attempted fix for #246, which was first noticed in #240. With some [trial and error](#240 (comment)) I set a precision of `.10g` (10 significant figures in exponential/scientific notation) for floating point numbers. I also had to change the `gdxdiff` options to additionally use a tolerance in relative difference when comparing the output DD files to the ground truth, otherwise the higher precision output above increased the DD diff. We now use `1e-6` for both absolute `Eps` and relative `RelEps` difference tolerance. If I understand correctly how `gdxdiff` [works](https://www.gams.com/latest/docs/T_GDXDIFF.html#GDXDIFF_OPTIONS_CRITERIONEXPLANATION), this feels like a reasonable tolerance: ``` AbsDiff := Abs(V1 - V2); if AbsDiff <= EpsAbsolute then Result := true else if EpsRelative > 0.0 then Result := AbsDiff / (1.0 + DMin(Abs(V1), Abs(V2))) <= EpsRelative else Result := false; ``` Finally, we were reporting the number of lines in the output of `gdxdiff`, but that's only the number of sets/parameters that are different, and doesn't track improvements in number of rows different. I'm now passing the output of `gdxdiff` to `gdxdump` so that the "GDX Diff" column is more accurate and tracks number of rows in the diff.
|
Looks like #251 would push this over the line! |
|
Looks like at least 4 additional records need #255 to be counted as correct. :-) |
|
@siddharth-krishna, this is now ready for a review! |
| "module_name", | ||
| ] | ||
| df.dropna(subset="value", inplace=True) | ||
| drop_cols = [col for col in df.columns if col != "value" and col not in keep_cols] |
There was a problem hiding this comment.
keep_cols should be a set so that the not in check is fast. We can use list(keep_cols) below if drop_duplicates requires a list, and do an O(n) conversion to a list once, instead of an O(n) list membership check for each column here.
xl2times/datatypes.py
Outdated
| @@ -266,6 +267,20 @@ def model_years(self) -> set[int]: | |||
| """ | |||
| return self.past_years | set(self.time_periods["m"].values) | |||
|
|
|||
| @cached_property | |||
| def veda_cgs(self) -> dict[tuple[str, str, str], str]: | |||
| """veda_cgs is a dictionary mapping commodities to their Veda commodity groups.""" | |||
There was a problem hiding this comment.
| """veda_cgs is a dictionary mapping commodities to their Veda commodity groups.""" | |
| """A dictionary mapping commodities to their Veda commodity groups.""" |
Just double checking: we don't expect this value to change when other properties/fields of the dataclass changes? Maybe we should add a TODO to check when this cached property is invalidated and have a way to clear the cache / recompute this. I'm wondering if that will be useful once we allow people to work with TimesModel objects directly in e.g. Jupyter Notebooks?
There was a problem hiding this comment.
Sounds like a good idea!
No description provided.