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

cable_global_properties.catalogue assignment changes iteration order, is surprising #1879

Closed
Helveg opened this issue Apr 8, 2022 · 4 comments · Fixed by #1882
Closed

cable_global_properties.catalogue assignment changes iteration order, is surprising #1879

Helveg opened this issue Apr 8, 2022 · 4 comments · Fixed by #1882
Labels

Comments

@Helveg
Copy link
Collaborator

Helveg commented Apr 8, 2022

Describe the bug
When assigning to the catalogue property of a cable_global_properties object, it seems to create and store a copy of the original object, which has some surprising side effects: it invalidates and shuffles the otherwise seemingly deterministic order of the mech cat iterator:

>>> list(arbor.load_catalogue("lib.so"))
["A", "B", "C", "D"]
>>> list(arbor.load_catalogue("lib.so"))
["A", "B", "C", "D"]
>>> list(arbor.load_catalogue("lib.so"))
["A", "B", "C", "D"]
>>> cat = arbor.load_catalogue("lib.so"); print(list(cat)); print(list(cat)); print(list(cat))
["A", "B", "C", "D"]
["A", "B", "C", "D"]
["A", "B", "C", "D"]
>>> props = arbor.cable_global_properties(); props.catalogue = cat; print(list(props.catalogue))
["C", "B", "D", "A"]

Context
Whilst @thorstenhater will probably argue that it is a bag, or a set, or a dragon, and order is not guaranteed 😉 it is still an open trapdoor since users (read: me (and @noraabiakar )) build up an assumption through use of the catalogues and its iterators, that the order is preserved. When then accessing the "same" catalogue through recipe.properties.catalogue for example, another iteration order is obtained. This led to catastrophy in the benchmarks, where the recorded samples and mechanism label were mismatched when zipped together.

Please do not propose to shuffle the return results of the mech cat iterator 👉 👈

@Helveg Helveg added the bug label Apr 8, 2022
@Helveg Helveg changed the title cable_properties.catalogue assignment creates copy, and is surprising cable_global_properties.catalogue assignment creates copy, and is surprising Apr 8, 2022
@Helveg Helveg changed the title cable_global_properties.catalogue assignment creates copy, and is surprising cable_global_properties.catalogue assignment changes iteration order, is surprising Apr 8, 2022
@thorstenhater
Copy link
Contributor

Hi,
before #1807 we had a reference (pointer) that needed to be kept alive, now we copy. What you are iterating over is
the set of keys of a map, not a dragon. I agree that the change in order is a bit surprising. Essentially the mech_cat_iterator is a view onto the key set of the underlying std::unordered_map. However, we do not construct
that map by copy but via import which iterates through the source's keys and inserts them one by one. This
changes the order, eg
https://gcc.godbolt.org/z/b6dWKKe8j

That said, never rely on

  • key order in unordered containers (python's set/map included)
  • the validity of one iterator in another container

The solution should be to use sorted(keys) in both cases?!

@Helveg
Copy link
Collaborator Author

Helveg commented Apr 11, 2022

What can be done to make this less surprising then? Can we sort the iteration on the CPP side? Alphabetic ordering of the mechs seems like a good user experience.

@thorstenhater
Copy link
Contributor

thorstenhater commented Apr 12, 2022

Have you tried using

for m in sorted(cat):
  print(m)

For me that gives a alpha-sorted list

In [1]: import arbor as A
In [2]: c = A.default_catalogue()
In [3]: [m for m in c]
Out[3]:
['exp2syn',
 'decay',
 'expsyn_stdp',
 'nax',
 'nernst',
 'kamt',
 'kdrmt',
 'expsyn',
 'gj',
 'hh',
 'pas',
 'inject']
In [4]: [m for m in sorted(c)]
Out[4]:
['decay',
 'exp2syn',
 'expsyn',
 'expsyn_stdp',
 'gj',
 'hh',
 'inject',
 'kamt',
 'kdrmt',
 'nax',
 'nernst',
 'pas']

@Helveg
Copy link
Collaborator Author

Helveg commented Apr 12, 2022

I'm sure that works, but should a user have to know to have to do that? However you make a user aware of this, it's an additional burden, and software rife with things like this is not fun to use.

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

Successfully merging a pull request may close this issue.

2 participants