-
Notifications
You must be signed in to change notification settings - Fork 17
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
Fix writing of default values #216
base: master
Are you sure you want to change the base?
Conversation
@trevorb1 - we have a performance problem. Creating new pandas dataframes with default values, concatenating the read-in values, and deleting duplicate values is slow and memory intensive. We need to consider alternatives.
|
Thanks for the feedback, @willu47! I looked at this today and think your option 2 of using xarray will be the way forward. I refactored the current code a little, just to make use of instance variables. My first thought of using pandas This method still wont work with bigger models, though :( I was using OSeMSOYS Global to generate generic input data for these tests. What I found was that anything past a medium size model, and my computer just killed it after like 45sec (I have a mid-range laptop). imo, this makes the I will look at the xarray implementation to see if that works better! Thanks for the idea on that! |
With the latest updates I have (following @willu47's suggestion here):
Still to do before reviewing/merging this:
I guess the scope of this PR has changed from "fixing" writing defaults to just refactoring it in otoole to address issue #217. Based on my comment above, seems like the actual fixing will really only come with the xarray implementation, as mentioned by @willu47 here (which relates to the open PR #184) |
@@ -152,8 +152,7 @@ def _write_parameter( | |||
default : int | |||
""" | |||
|
|||
if not self.write_defaults: | |||
df = self._form_parameter(df, default) | |||
df = self._form_parameter(df, default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is where datafile defaults are being filtered out, even if write_defaults
is passed. The issue is that write_defaults
is no longer exposed to the write strategy, so we need a way to address this conditional.
Okay, I think writing defaults has been moved to
A few notes:
|
Hi @trevorb1, I just ran a test with |
@HauHe Great! To confirm, your test was for a multi-year model, right? I cant remember if another issue arose for a single year model that relates to this. But maybe since that is more of an edge case, deserves its own issue ticket |
@trevorb1, yes the model I used for testing (UTOPIA) has multiple years. |
I ran a test with one of my models, and there is no data in the |
@trevorb1, thanks for yesterday's discussion. I tried to install the version from this branch without success; the
For now, the easiest solution is to un-hash |
Thanks for the the update @robertodawid! Do you have a minimal datafile + model you would be able to attach that I would be able to recreate this issue with? |
Description
In this PR I have fixed the writing out of default values through the existing
--write_defaults
flag. Accompanying tests have been added.This functionality existed (and worked) for writing out parameter defaults, but did not for writing out result defaults. This was due to the input data not being passed into the
WriteStrategy._expand_defaults(...)
function. I have addressed this, however, naming of variables is not super clear now in the WriteStrategy class.Within
WriteStrategy
, data to be written out is now calledinputs
(this can be set/param data or result data), while input data for the model (only param/set data) is now calledinput_data
. idk, we may want to clarify this before merged, or it may just be fine since its only in one place and documented in the docstring of_expand_defaults(...)
and at variable definition.Issue Ticket Number
My suggestion is for this PR to replace PR #215
Documentation
The functionality was already documented, but it didnt work!