Fix some issues #252
Fix some issues #252
Conversation
pysd/py_backend/functions.py
Outdated
| # dictionary to store the values from SAMPLE IF TRUE function | ||
| saved_value = {} | ||
|
|
||
| def make_da(rows, cols, initial_value): | ||
| """ | ||
| Returns a DataArray with the coordinates | ||
| of the rows and cols. | ||
| DataArray values are initialized with the initial_value. | ||
| It is used in SAMPLE IF TRUE function, to create the proper dimension saved value. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| rows: float or xarray.DataArray | ||
| Represents the row dimension of the new DataArray | ||
| cols: xarray.DataArray | ||
| Represents the col dimension of the new DataArray | ||
| initial_value: float or xarray.DataArray | ||
| Include the values to initialize the new DataArray | ||
|
|
||
| Returns | ||
| ------- | ||
| A new DataArray with proper rows and cols coordinates, | ||
| initialized with initial_value | ||
|
|
||
| """ | ||
| if(isinstance(initial_value,xr.DataArray)): | ||
| array = np.array([[initial_value.data[e] for i in range(0, len(rows.values))] for e in range(0,len(cols.values))]) | ||
| elif(isinstance(rows, xr.DataArray)): | ||
| array = np.array([[initial_value for i in range(0, len(rows.values))] for i in range(0,len(cols.values))]) | ||
| else: | ||
| array = np.array([initial_value for i in range(0,len(cols.values))]) | ||
|
|
||
| coords = {dim: cols.coords[dim] for dim in cols.dims} | ||
| dims = cols.dims | ||
| if(isinstance(rows, xr.DataArray)): | ||
| coords.update({dim: rows.coords[dim] for dim in rows.dims}) | ||
| dims += rows.dims | ||
| return xr.DataArray(data=array, coords=coords, dims=dims) | ||
|
|
||
| def sample_if_true(time, condition, actual_value, initial_value, var_name): | ||
| """ | ||
| Implements Vensim's SAMPLE IF TRUE function. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| condition: bool or xarray.DataArray | ||
| actual_value: float or xarray.DataArray | ||
| Value to return when condition is true. | ||
| initial_value: float or xarray.DataArray | ||
| Value to return when condition is false. | ||
| var_name: str | ||
| Represents the SAMPLE IF TRUE function in the whole model. | ||
|
|
||
| Returns | ||
| ------- | ||
| float or xarray.DataArray | ||
| Actual_value when condition is true and saved this value | ||
| in saved_value dictionary. | ||
| Returns the last saved value when condicion is false. | ||
| Saved value is initialized with initial_value in the first step of simulation. | ||
| """ | ||
| global saved_value | ||
| t = time() | ||
|
|
||
| if(t==0): | ||
| if(not(isinstance(condition,xr.DataArray))): | ||
| saved_value[var_name] = initial_value | ||
| else: | ||
| saved_value[var_name] = make_da(actual_value, condition, initial_value) | ||
|
|
||
| if isinstance(condition, xr.DataArray): | ||
| if condition.all(): | ||
| for i in range(0,len(saved_value[var_name].values)): | ||
| saved_value[var_name].values[i]=actual_value | ||
| return saved_value[var_name] | ||
| elif not condition.any(): | ||
| return saved_value[var_name] | ||
|
|
||
| for i in range(0, len(condition)): | ||
| if condition.values[i]: | ||
| saved_value[var_name][i]=actual_value | ||
| return xr.where(condition, actual_value, saved_value[var_name]) | ||
|
|
||
| if condition: | ||
| saved_value[var_name] = actual_value | ||
|
|
||
| return saved_value[var_name] |
There was a problem hiding this comment.
I suspect that you didn't notice that in the test for the sample_if_true model I indicated that I was working on it.
As this function requires an initial value and information about a saved value, it seems reasonable to implement it as a stateful object. In fact, I have tested an implementation of SampleIfTrue as a Stateful object which requires less code and is more consistent with the PySD structure.
I will be able to make a PR by next week.
There was a problem hiding this comment.
Hi @enekomartinmartinez,
I had not seen that you were working on sample if true function until I looked up if there was any test for this function. By then, I had had a good part of implementation done yet.
Maybe, they could add my function to the repo, because this implementation for sample if true works, and then, when you make your pull request, change and improve the sample if true function.
In addition, I take the opportunity to ask you about what are you working on? Or what can I work on to make progress?
There was a problem hiding this comment.
Hi @marrobl,
Sorry for taking so long to answer.
The problem of changing the sample_if_true function by a stateful object SampleIfTrue is that we will make a non-backward compatible version. We need also to make the initialization of the function solid, in the case of using a Stateful object this is simple as we have the initialize method. Note that your function may not be well initialized if initial_time is not 0.
You can incorporate the SampleIfTrue inherited from Satetful class, use a similar builder like the one used for Integ or Delay as it makes it possible to work with subscripted variables. The initialization of the object could be similar to the one done with Integ. You can create a call method that evaluates the condition with the current if_then_else function and updates the state if needed. As was done with the Initial class, it may be necessary to add a ddt equal to 0 and pass when update is call.
There was a problem hiding this comment.
Hi @enekomartinmartinez,
Okey, I'm on it. I'm going to try to develop sample if true function the way you have told me. Hope to have it finished soon so I can include in this pull request
There was a problem hiding this comment.
I have already added the sample if true functionality as stateful object.
I have not been able to use the if_then_else function due to the type of arguments (functions) and the type of results of that function (simple values). This did not allow updating the state of the object and, at the same time, using it as a parameter of the if_then_else_function.
So, I have had to create a new function similar to if_then_else.
There was a problem hiding this comment.
Hi @marrobl, the SampleIfTrue class looks very nice :)
Yes, if_then_else takes the val_if_true and val_if_false as functions just to use lazy evaluation as VENSIM does. Have you tried instead using a lambda function in self.state this will solve the problem without needing to create a new function:
self.state = if_then_else(self.condition(), self.actual_value, lambda: self.state)
if you are able to check that I think we can already merge this pull request.
There was a problem hiding this comment.
You're right @enekomartinmartinez! Thanks, I did not realize how to 'cast' a simple value to a function.
pysd/py_backend/vensim/vensim2py.py
Outdated
| for i in range(len(subs_start)): | ||
| if(subs_start_l[i] == subs_end_l[i] and not(subs_start_l[i].isdigit())): | ||
| common = common + subs_start_l[i] | ||
| else: break |
There was a problem hiding this comment.
This may crash for a range defined as (Y2020M1-Y2020M12).
Using regex as shown below would prevent that:
subs_start = re.findall('\d+|\D+', subs_start)
subs_end = re.findall('\d+|\D+', subs_end)
prefix = ''.join(subs_start[:-1])
num_start = int(subs_start[-1])
num_end = int(subs_end[-1])
There was a problem hiding this comment.
Yes, good point, thank you!
I have changed the grammar and the function and now it works in that case.
There was a problem hiding this comment.
Perfect, you're welcome :)
pysd/py_backend/vensim/vensim2py.py
Outdated
| def visit_range(self, n, vc): | ||
| subs_start = vc[2].strip() | ||
| subs_end = vc[6].strip() | ||
| self.sequence, start, end = get_subscript_number_range(subs_start, subs_end) |
There was a problem hiding this comment.
As the get_subscript_number_range function is only used once by visit_range, I would add the code here directly not making a separate function.
There was a problem hiding this comment.
I see your point but by leaving the function out of the visitor, I intend to simplify the complexity of the visitor code, following the principle: “When you feel the need to write a comment, first try to refactor the code so that any comment becomes superfluous." ( Robert C. Martin's in "Clean code").
So, in my opinion, it is clearer and simpler to split it into a function with its documentation even though this function is only used once.
There was a problem hiding this comment.
I hear you, however, the structure of vensim2py is already complicated to follow. In other cases, unless functions from the builder are required, the necessary changes are done in place. I think this keeps the general structure of the vensim2py cleaner, nevertheless, the visitors are more loaded.
I leave it to @JamesPHoughton to decide wether to use a separate function or not.
There was a problem hiding this comment.
This is a tricky one. The parser is a complicated piece of code, and I'm not sure of the best strategy for keeping it manageable.
On the one hand, there are certainly going to be bits that are complex enough to be their own functions, even if they are only used once, and @marrobl's function documentation is super clean. On the other hand, the rest of the file localizes the code needed by the visitor in the visitor itself, and it's also going to be easier to understand code that picks a strategy and sticks to it.
In the particular case of the parser, the style consistency argument is persuasive to me. I think putting the code all together in the visitor might be the best course.
TBS, if we were going to rewrite the parser from scratch, or develop a brand new piece of code for PySD, I'd probably want to decompose things more and document functions better. This is the unfortunate legacy of the original developer not being a real software developer. ;) Sorry 'bout that...
There was a problem hiding this comment.
Okay, thank you both. I already put the code in the visitor
pysd/py_backend/vensim/vensim2py.py
Outdated
| arguments = [] | ||
| while len(','.join(arguments)) < len(vc[4]): | ||
| arguments.append(self.args.pop()) | ||
| arguments = [arguments[-1]] + arguments[:-1] |
There was a problem hiding this comment.
There was a problem hiding this comment.
You are right. My error, I had not seen the issue #248 solved before I made this pull request.
| numeric_range = _ (range / value) _ ("," _ (range / value) _)* | ||
| value = _ sequence_id _ | ||
| range = "(" _ sequence_id _ "-" _ sequence_id _ ")" |
There was a problem hiding this comment.
The implementation of numeric_range seems good :)
|
Hi, or |
pysd/py_backend/vensim/vensim2py.py
Outdated
| if(not(prefix_start) or not(prefix_end)): raise ValueError('A numeric range must contain at least one letter') | ||
| if(num_start>num_end): raise ValueError('The number of the first subscript value must be lower than the second subscript value in a subscript numeric range\n') | ||
| if(prefix_start != prefix_end or subs_start[0].isdigit() or subs_end[0].isdigit()): raise ValueError('Only matching names ending in numbers are valid') |
There was a problem hiding this comment.
This seems nice :)
I have had some headaches about how to give human-readable errors when something breaks inside the parser.
As when an error is raised parsimonious usually rises own errors, which sometimes are difficult to follow and remove the information of what failed, this is more common in parse_general_expression parser.
Have you tried raising these errors when parsing a file?
I added some more informative messages in the parser in PR #249 (see lines 347-359 and 1121-1134 from vensim2py). We may need to see how to incorporate all error raises together so they are easy to identify.
There was a problem hiding this comment.
Yes, I have tried raising these errors when parsing a file.
I am going to add more information in these error messages so that the user can identify in which numeric range name the error is found in more easily.
… the function get_subscript_numeric_range and the code has been added to the visitor
|
I have just forgotten, could you update the version to 1.3.0 in |
Add update method to SampleIfTrue with a pass, so when trying to update SampleIfTrue nothing will be done. Improves reliability and speed.
|
Thanks @marrobl! |
In this pull request, I added SAMPLE IF TRUE function. Issue #217. Also, I uncommented the sample if true test in the vensim integration file.
I added subscript numeric range support and a test to check it. Issue #251. So I make another pull request in test models repository to add a test model for subscript numeric range.
I changed the way to split arguments in visit_call function in expression grammar (line 961). In this way it is possible to define a function as a parameter of another. Solving issue #250. This way of splitting arguments had been done in julienmalard#1 but, I do not why, it was not finally added to PySD.