Open brocksam opened 3 years ago
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 small example to reproduce the issue is :
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:
The dependent variables will be ordered alphabetically in the returned namedtuple()
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. I wouldn't hate the change, but I'm afraid it will break too many user scripts to do without a major version increase and I'm not sure that's worth it.
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".