(refactor) learner.tell(x, None) might be renamed to learner.tell_pending(x)

[basnijholt]

(original issue on GitLab)

opened by Jorn Hoofwijk (@Jorn) at 2018-07-01T13:35:58.127Z

This issue is a result of a discussion we had here: https://gitlab.kwant-project.org/qt/adaptive/merge_requests/68#note_15204

I suggested splitting learner.tell(x, value) into two functions,

1. one with value=None: make this learner.tell_pending(x) as this method to me seems more intuitive
2. one with value!=None: keep this the same as it was before

This change will be a breaking change to the api, however we could accept the old implementation for a certain amount of time (e.g. a few months) and raise a DeprecationWarning in the meantime.

While we're at it, we may also change some other functions,
@jbweston also suggested having learner.peek(n) instead of learner.ask(n, add_data=False). This falls in a similar category.

Other naming suggestions than learner.tell_pending and learner.peek are also welcome.

Lastly I would also really enjoy not passing real=True/False to any method, in the beginning I had a hard time understanding the code for this reason, and most functions had an if statement to check if real was True or False, so in essence it is just two functions merged into one.

[basnijholt]

originally posted by Joseph Weston (@jbweston) at 2018-07-02T18:46:01.120Z on GitLab

I agree that this is the way to go.

Separating out functionality that is conceptually different is good for readability.

[basnijholt]

originally posted by Joseph Weston (@jbweston) at 2018-07-02T18:51:54.460Z on GitLab

Also, looking at BalancingLearner again, I see that for it to do its job it needs the concept of "cancelling" a pending datapoint.

Presently we hack around this by saving and resetting the state of all the child learners, but if the child learner's provided cancel_pending this would be much cleaner.

Of course, cancelling outstanding points is probably nearly as expensive as adding points. At the very least we need to downdate a bunch of internal datastructures. e.g. for the 2D learner I could imagine one may need to resort to rebuilding the whole triangulation

[basnijholt]

originally posted by Jorn Hoofwijk (@Jorn) at 2018-09-25T11:56:18.990Z on GitLab

This is partially solved in the current master, that is: tell(x, None) is replaced with tell_pending(x, None). ask however remained similar, with ask(n, add_data=True/False) becoming ask(n, tell_pending=True/False)

[basnijholt]

originally posted by Bas Nijholt (@basnijholt) at 2018-09-26T12:30:03.720Z on GitLab

I think this issue can be closed.

The discussion of splitting

• learner.ask(n, tell_pending=True) -> learner.ask(n)
• learner.ask(n, tell_pending=False) -> learner.peek(n)

(with which I dissagree) can be in a seperate issue.