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

AverageLearner1D added #283

Merged
merged 51 commits into from Aug 27, 2021
Merged

AverageLearner1D added #283

merged 51 commits into from Aug 27, 2021

Conversation

AlvaroGI
Copy link
Contributor

@AlvaroGI AlvaroGI commented Jun 19, 2020

The AverageLearner1D has been added to the master branch (original code with design details and tests can be found here).
Tutorial notebook added (tutorial_averagelearner1d_aux.py contains some auxiliary functions for the tutorial).

Changes in existing files

The init.py files of adaptive/ and learner/, and notebook_integration.py were only modified to include the AverageLearner1D.

Copy link
Member

@basnijholt basnijholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great! I am super excited to use this 🎉

I have made some style changes and I left some small comments.

Todo:

  • add tests
  • add tutorial in the right format

adaptive/learner/averagelearner1D.py Outdated Show resolved Hide resolved
adaptive/learner/averagelearner1D.py Outdated Show resolved Hide resolved
adaptive/learner/averagelearner1D.py Outdated Show resolved Hide resolved
adaptive/learner/averagelearner1D.py Outdated Show resolved Hide resolved
@basnijholt
Copy link
Member

Awesome!
AverageLearner1D mov

AlvaroGI and others added 19 commits March 23, 2021 15:11
The AverageLearner1D has been added to the master branch (two changes were made: Learner1D._update_data_structures() does not exist anymore, so we modified AverageLearner1D accordingly; also, self._data was changed to self.data).
Tutorial notebook added (tutorial_averagelearner1d_aux.py contains some auxiliary functions for the tutorial).
The __init__.py files of adaptive/ and learner/, and notebook_integration.py were modified to include the AverageLearner1D.
The old version was bugged (it rewrote learner data and raised errors when ran repeatedly). The new version worked well in all tests we perfomed.
Doc-string also fixed.
This method now matches the definition from the BaseLearner. It provides a computational efficiency in some scenarios (see the comments in the code), otherwise it just performs a loop with a tell(x,y)
The docs and tutorial have been updated to include the AverageLearner1D. The previous tutorial (Python notebook) has been replaced by new tutorial as .rst file.
AlvaroGI and others added 5 commits March 31, 2021 17:13
The new value of the data at point adopted the value of the mean of the new data samples, instead of the mean over all samples (new and old). Fixed!
@basnijholt
Copy link
Member

When testing this I realized that the function is not deterministic (does not take a seed).

I am implementing that the function signature of the function-to-learn is: f(x_seed: Tuple[int, float]).

@akhmerov
Copy link
Contributor

When testing this I realized that the function is not deterministic (does not take a seed).

Well spotted, thanks!

@basnijholt
Copy link
Member

I am fixing the remaining (2) failing tests.

test_expected_loss_improvement_is_less_than_total_loss is failing because the loss_improvement for seed=0 is always set to np.inf and afterwards to 0.

e.g.:

learner.ask(10)

returns

([(0, -2),
  (1, -2),
  (2, -2),
  (3, -2),
  (4, -2),
  (5, -2),
  (6, -2),
  (7, -2),
  (8, -2),
  (9, -2)],
 [inf, 0, 0, 0, 0, 0, 0, 0, 0, 0])

for an empty learner.

With data:

With data it is:

([(40, -2),
  (41, -2),
  (42, -2),
  (43, -2),
  (44, -2),
  (45, -2),
  (46, -2),
  (47, -2),
  (48, -2),
  (49, -2)],
 [0, 0, 0, 0, 0, 0, 0, 0, 0, 0])

@basnijholt
Copy link
Member

I have fixed all the tests! 🎉

I have also enabled auto-merge. It's now in a working state, we can make more changes in subsequent PRs.

@AlvaroGI, really great work!

@basnijholt basnijholt merged commit ea01877 into python-adaptive:master Aug 27, 2021
@AlvaroGI
Copy link
Contributor Author

I have fixed all the tests! 🎉

I have also enabled auto-merge. It's now in a working state, we can make more changes in subsequent PRs.

@AlvaroGI, really great work!

That’s great news!! 🎉 Thank you for all your work, it looks so neat now! 😄

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

Successfully merging this pull request may close these issues.

None yet

3 participants