search

python-adaptive / adaptive

:chart_with_upwards_trend: Adaptive: parallel active learning of mathematical functions
http://adaptive.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
1.14k stars 58 forks source link

Make learnerND datastructures immutable where possible #54

Closed basnijholt closed 5 years ago

basnijholt commented 5 years ago

(original issue on GitLab)

opened by Joseph Weston (@jbweston) at 2018-07-10T13:16:25.518Z

At the moment there are a few datastructures that are described as "sets". where possible we should make these frozenset to remove the possibility of them being modified.

basnijholt commented 5 years ago

originally posted by Jorn Hoofwijk (@Jorn) at 2018-07-13T11:46:10.397Z on GitLab

Wauw, according to this thread , a frozenset is hashable and can therefore be used as a dictionary key. Maybe this is a clean way to store simplices (rather than having a sorted tuple) in the triangulation/learner.

any opinions?

basnijholt commented 5 years ago

originally posted by Joseph Weston (@jbweston) at 2018-07-13T11:49:40.261Z on GitLab

In [1]: import sys

In [2]: sys.getsizeof((1, 2, 3))
Out[2]: 72

In [3]: sys.getsizeof(frozenset((1, 2, 3)))
Out[3]: 224

just saying

basnijholt commented 5 years ago

originally posted by Jorn Hoofwijk (@Jorn) at 2018-07-13T11:50:33.059Z on GitLab

ohw, that is a shame

basnijholt commented 5 years ago

originally posted by Joseph Weston (@jbweston) at 2018-07-13T11:50:41.004Z on GitLab

because a frozenset is a hashmap, whereas a tuple is an array

basnijholt commented 5 years ago

originally posted by Bas Nijholt (@basnijholt) at 2018-12-07T20:00:10.457Z on GitLab

Will this ever be implemented? If not, can we close it?

basnijholt commented 5 years ago

originally posted by Anton Akhmerov (@anton-akhmerov) at 2018-12-07T20:10:46.447Z on GitLab

It's ~"priority: low", so please let's not close it. @jbweston's illustration shows clearly that there's a clear benefit in implementing this issue.

basnijholt commented 5 years ago

originally posted by Joseph Weston (@jbweston) at 2018-12-08T09:53:44.558Z on GitLab

@jbweston's illustration shows clearly that there's a clear benefit in implementing this issue.

No, I was just saying that we should prefer tuples over sets where possible, but that some of the structures that currently are sets should be frozenset if they can be

basnijholt commented 5 years ago

originally posted by Anton Akhmerov (@anton-akhmerov) at 2018-12-08T20:25:06.809Z on GitLab

Indeed, I misunderstood.

Looking at the documentation of LearnerND I'm not sure what data structures would benefit from conversion.

On the other hand, it does seem like we could make a bunch of methods private and therefore make the API simpler.

basnijholt commented 5 years ago

originally posted by Bas Nijholt (@basnijholt) at 2018-12-11T11:56:04.313Z on GitLab

Like this @anton-akhmerov? https://gitlab.kwant-project.org/qt/adaptive/merge_requests/142