
changed the description
Toggle commit list 
Maintainer
I just discovered that this doesn't fix the issue actually.
It still happens...


Maintainer

Maintainer
I think that we can remove all other places where we do
abs(dx) > self._dx_eps else 0
because such intervals should never appear anyways. 
Toggle commit list

added 1 commit
 55b3c216  1D: remove unneeded function and change to a normal loop
Toggle commit list 
added 1 commit
 2e3f5793  1D: remove unneeded function and change to a normal loop
Toggle commit list 
Maintainer
Just to try to convince you, I am calculating the following plot:
Without having both
_dx_eps *= 2 * _dx_eps_old
and the new check inask
that plot fails within 1000 points with aKeyError
, but with these two fixes it continues >100_000
(after which I stopped it.) 

Toggle commit list

Maintainer
Another reason why the current implementation is incorrect:
def f(x): return 1 if x == 0 else 0 learner = adaptive.Learner1D(f, bounds=(1, 1)) ex = adaptive.runner.SequentialExecutor() runner = adaptive.BlockingRunner(learner, executor=ex, goal=lambda l: l.loss() < 0.001, log=True) learner.loss()
0.0009765625
Even though it did found
f(x=0)=1
. The loss is so small becauselearner.losses[(2.220446049250313e16, 0.0)] == 0.0
even thoughlearner.data[0.0] == 1
andlearner.data[2.220446049250313e16] == 0
. 
Maintainer
On the other hand it actually makes sense to assign
0
loss to that interval because that interval has no chance of ever improving...So maybe we do need to keep the old code too...
To be honest I am a bit lost too now
😂 

added 28 commits

5f66df94...ff8bc9b6  22 commits from branch
master
 32e931c5  1D: increase _dx_eps
 9166f810  1D: also check for abs(dx) < self._dx_eps in ask
 c3d3c32d  1D: remove unneeded checks for _dx_eps
 8a870796  1D: remove unneeded function and change to a normal loop
 5fe345c4  1D: add a comment and redifine dx > dx
 00b68a8b  improve code homogeneity
Toggle commit list 
5f66df94...ff8bc9b6  22 commits from branch

Maintainer
Not a very minimal example (I couldn't reduce it further) but the code below hits the issue each time after 500700 points at a machine with 32 cores. With the fixes in this PR it works until 288.587 points (where I stopped it).
import adaptive import kwant import kwant.continuum import scipy.constants adaptive.notebook_extension() def get_template(subs): ham = ("(0.5 * hbar**2 * k_x**2 / m_eff  mu + V) * sigma_z + Delta * sigma_x") return kwant.continuum.discretize(ham, locals=subs) def make_wire(): syst = kwant.Builder() template = get_template(subs={'Delta': '0'}) syst.fill(template, lambda site: site.pos[0] == 0, (0,)) symmetry = kwant.TranslationalSymmetry((1,)) lead = kwant.Builder(symmetry) template = get_template(subs={'V': '0'}) lead.fill(template, lambda site: True, (0,)) syst.attach_lead(lead) return syst.finalized() # (Fundamental) constants definitions in nm and meV. constants = { 'm_eff': 0.015 * scipy.constants.m_e / scipy.constants.eV / (1e9)**2 * 1e3, 'hbar': scipy.constants.hbar / scipy.constants.eV * 1e3, } params = dict(Delta=1, mu=10, V=1, **constants) syst = make_wire() def G(E, syst=syst, params=params): return kwant.smatrix(syst, E, params=params).transmission(0, 0) learner = adaptive.Learner1D(G, (0.99, 1.01)) runner = adaptive.BlockingRunner(learner, goal=lambda l: l.npoints > 10000)

Maintainer
OK more minimal:
def f(x): import random return 0 if x < 1 else 1 + 10**(random.randint(12, 14)) learner = adaptive.Learner1D(f, (0, 2)) runner = adaptive.Runner(learner)
the following also works, but it needs much more points before it fails.
def f(x): import random return 10**(random.randint(12, 14))
parallelism seems to be needed to make the
Runner
fail. 

Maintainer
@jbweston and/or @antonakhmerov could you have a look at this? I think it's ready to merge.
I've added the relevant tests that would fail without my changes.

added 12 commits

4f2e8457...83d5722d  2 commits from branch
master
 64320f5c  1D: increase _dx_eps
 23c5bf3e  1D: also check for abs(dx) < self._dx_eps in ask
 53a93e3f  1D: remove unneeded checks for _dx_eps
 d5279eb6  1D: remove unneeded function and change to a normal loop
 943b44cc  1D: add a comment and redifine dx > dx
 345b778c  improve code homogeneity
 7be9f7fa  set the actual loss to zero when the interval is smaller than machine precision
 dcbfe8f6  add tests for small intervals
 c739d04c  fix bugs in tests
 73e5ac88  remove metadata from notebook
Toggle commit list 
4f2e8457...83d5722d  2 commits from branch

Owner
I am confused: can you please explain how this changeset eliminates a
KeyError
?Importantly a
KeyError
means that are transforming the data somewhere, and we should never do so with dictionary keys. 
Maintainer
I don't know why the error happens, @jbweston looked at it too and also didn't understand IIRC.
I do know that with these changes the error never happens anymore.

Owner
If you don't understand what the original error was, how do you know that your tests cover that failure case?
Additionally I observe that your tests are using a multiprocessing queue, and they are therefore not reproducible. This completely disqualifies them as tests.

Owner
Also
test_loss_at_machine_precision_interval_is_zero
passes on master. 
Owner
Most importantly: a
KeyError
of the kind reported in #61 (closed) must never occur, regardless of how we define loss and which points we allow to request. 
Maintainer
Also
test_loss_at_machine_precision_interval_is_zero
passes on master.The information is above (!73 (comment 14789)), but somewhat unclear maybe.
This is because before we were not avoiding intervals smaller than
_dx_eps
but only the children of those intervals will have zero loss and should therefore not be created.With my fix we ensure that there will never be intervals smaller than
_dx_eps
but not by setting the loss to zero. In that case onetest_small_deviations
was fixed but this introduced thattest_loss_at_machine_precision_interval_is_zero
would fail. This I also fixed by making sure that the loss of those small intervals is set to zero because we can't improve such an interval anymore. 

Owner
@basnijholt can you please update the test to not use a runner but instead to only sequentially send
ask
queries? Additionally should also also start right away with a tiny interval where points are e.g. 1, 1 + 4*eps. 
Maintainer
The problem doesn't occur doing it sequentially.

Maintainer
Additionally should also also start right away with a tiny interval where points are e.g. 1, 1 + 4*eps.
That doesn't seem to matter.


added 33 commits

73e5ac88...bb3e0b67  21 commits from branch
master
 fcbc8349  1D: increase _dx_eps
 3496db90  1D: also check for abs(dx) < self._dx_eps in ask
 6f32413c  1D: remove unneeded checks for _dx_eps
 d2466ad2  1D: remove unneeded function and change to a normal loop
 6269d947  1D: add a comment and redifine dx > dx
 0232132a  improve code homogeneity
 78a00f91  set the actual loss to zero when the interval is smaller than machine precision
 9a79ddd4  add tests for small intervals
 31b29630  fix bugs in tests
 65957cc9  remove metadata from notebook
 8d0120b2  1D: always return the points as a list
 060c1556  make the tests independent of the BlockingRunner
Toggle commit list 
73e5ac88...bb3e0b67  21 commits from branch

















resolved all discussions
Toggle commit list 
added 1 commit
 01a27a49  fix the bug where intervals could be smaller than dx_eps
Toggle commit list 
Maintainer
I finally pinpointed the error, we were avoiding intervals of size
< self._dx_eps
but then herefor point_number in range(n): quality, x, n = quals[0] heapq.heapreplace(quals, (quality * n / (n + 1), x, n + 1))
we are dividing them even further
for point_number in range(n): quality, x, n = quals[0] if abs(x[1]  x[0]) / (n + 1) <= self._dx_eps: # The interval is too small and should not be subdivided quality = np.inf heapq.heapreplace(quals, (quality * n / (n + 1), x, n + 1))
fixed it.
Now the test is still not fixed, but that is simply because it fails when:
(learner.bounds[1]  learner.bounds[0]) / learner.npoints < learner._dx_eps
at which point
learner.loss() == 0
(also because of changes in this MR).The problem is that when that condition is
True
, the learner essentially should not be able to return any more points. 
Owner
We should think how a learner would indicate that it's "done for good", related to #51, but more permanent.

added 1 commit
 cec1a802  fix 'test_small_deviations' by making it stop when loss==0
Toggle commit list 

Toggle commit list

Maintainer
@basnijholt just asked 1 more clarification, and once that's been closed merge.

added 145 commits

0ce8c3f2...d7f38957  130 commits from branch
master
 83dcfe11  1D: increase _dx_eps
 d7a9595f  1D: also check for abs(dx) < self._dx_eps in ask
 15b1d673  1D: remove unneeded checks for _dx_eps
 6951abea  1D: remove unneeded function and change to a normal loop
 ca73e605  1D: add a comment and redifine dx > dx
 1aefc887  improve code homogeneity
 306685cd  set the actual loss to zero when the interval is smaller than machine precision
 54dbcd84  add tests for small intervals
 64ee2d98  fix bugs in tests
 d97a48ca  1D: always return the points as a list
 b1af8b69  make the tests independent of the BlockingRunner
 8cd223fd  fix the bug where intervals could be smaller than dx_eps
 fe983dc0  fix 'test_small_deviations' by making it stop when loss==0
 4f2f4973  small style changes
 4f5bb634  change loop into list comprehension
Toggle commit list 
0ce8c3f2...d7f38957  130 commits from branch

resolved all discussions
Toggle commit list 
merged
Toggle commit list 
