-
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
Model
s do not necessarily return values in the same order in which they were supplied
#318
Comments
This behavior comes from sorting the model_dict alphabetically before storing it (https://github.com/tBuLi/symfit/blob/master/symfit/core/models.py#L323). a, b, x = variables("a, b, x")
m = Model({b : x**2, a : x}) #notice b before a
r_1, r_2 = m(x = np.array([2]))
print(r_1, r_2) #expect [2] [4]
>> [2] [4] It is not clear to me why model_dict needs to be sorted alphabetically. Removing the sort only breaks one test (https://github.com/tBuLi/symfit/blob/master/tests/test_model.py#L48), but it is not clear why this test is necessary. |
From https://symfit.readthedocs.io/en/stable/tutorial.html#named-models:
IIRC this was decided way back when dictionaries were not yet ordered by default, so a sane-ish default was chosen we could rely upon internally. I can't really pin down where this assumption is actually used at the moment though... No failing tests is a good sign. Note that the docs also say explicitly that accessing model results by variable name (a and b in this case) is the preferred way: https://symfit.readthedocs.io/en/stable/style_guide.html |
Test
tests/test_general::test_model_callable_from_model_init
is currently marked for skipping by pytest (this was done as part of PR #317) as its behaviour is not deterministic and therefore fails a proportion of the time. The reason for this is that theBaseModel._init_from_dict
method topologically sorts the supplied expressions for the model before initialising its internal mapping. The topological sorting will almost certainly reorder the expressions before storing them. When information is returned to the user by this class, the ordering of that information is based on the order of the mapping held byBaseModel
and therefore differs from the originally-supplied order. This discrepancy between supplied-order and returned-order is misleading behaviour and was highlighted as needing addressing in a review comment by @pckroon on PR #317.BaseModel
should be amended to also internally store the order in which the user supplied the arguments and this ordering should be used when returning data to the user. As stated by @pckroon, "the order in which we should return [the components] should be completely independent of what the topological order is".The text was updated successfully, but these errors were encountered: