Skip to content
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

Models do not necessarily return values in the same order in which they were supplied #318

Open
brocksam opened this issue Sep 23, 2020 · 2 comments

Comments

@brocksam
Copy link
Contributor

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 the BaseModel._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 by BaseModel 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".

@MaximeLucasSky
Copy link

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.

@pckroon
Copy link
Collaborator

pckroon commented Oct 16, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants