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

Remove initialize method from archives #200

Merged
merged 16 commits into from Jun 9, 2022
Merged

Remove initialize method from archives #200

merged 16 commits into from Jun 9, 2022

Conversation

eszabo12
Copy link
Contributor

@eszabo12 eszabo12 commented Jun 7, 2022

Description

I put the solution_dim (int): Dimension of the solution space. As the comment under args above the stuff. I put the solution_dim argument s.t. The new prototype for init in _archive_base is def init(self, cells, behavior_dim, solution_dim, seed=None, dtype=np.float64) for archive_base.
The new prototype for cvt_archive is def init(self,
cells,
ranges,
solution_dim,
seed=None,
dtype=np.float64,
samples=100_000,
custom_centroids=None,
k_means_kwargs=None,
use_kd_tree=False,
ckdtree_kwargs=None)
The new prototype for grid_archive is def init(self, dims, ranges, solution_dim, seed=None, dtype=np.float64)

I put solution_dim as a required (non-default) arg before seed in every instance.

TODO

  • Remove the archive.initialize() calls from the tests, because they are now deprecated and shouldn't work.
  • Comment line 61 in ribs/optimizers/_optimizer.py that calls the initialize function
  • Fix the comment in __init__.py that says ".. note:: After construction, each archive must be initialized by calling
    its initialize() method before it can be used"
  • Remove decorator from the install_requires in setup.py since it was only used for require_init

Questions

  • Are there any issues/unideal things about my changes to the documentation/commenting, etc
  • The output of pytest had a lot of issues, should I go through them systematically? i.e., there is an unused import on line 7 of _cvt_archive.py-- the require_init function.

Status

  • I have read the guidelines in CONTRIBUTING.md.
  • I have formatted my code using yapf
  • I have tested my code by running pytest
  • I have linted my code with pylint
  • I have added a one-line description of my change to the changelog in HISTORY.md.
  • This PR is ready to go

@eszabo12 eszabo12 requested a review from btjanaka June 7, 2022 01:33
Copy link
Member

@btjanaka btjanaka left a comment

Choose a reason for hiding this comment

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

Make sure to remove the call to initialize in the Optimizer, and remove/fix any tests. Also, try running the example code from the pyribs home page to make sure everything still works.

ribs/archives/_archive_base.py Outdated Show resolved Hide resolved
ribs/archives/_archive_base.py Outdated Show resolved Hide resolved
ribs/archives/_archive_base.py Show resolved Hide resolved
ribs/archives/_archive_base.py Outdated Show resolved Hide resolved
ribs/archives/_archive_base.py Outdated Show resolved Hide resolved
Copy link
Member

@btjanaka btjanaka left a comment

Choose a reason for hiding this comment

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

Minor changes

examples/lunar_lander.py Outdated Show resolved Hide resolved
ribs/archives/_archive_base.py Outdated Show resolved Hide resolved
ribs/archives/_archive_base.py Outdated Show resolved Hide resolved
ribs/archives/_cvt_archive.py Outdated Show resolved Hide resolved
ribs/archives/_cvt_archive.py Outdated Show resolved Hide resolved
ribs/archives/_cvt_archive.py Outdated Show resolved Hide resolved
ribs/archives/_grid_archive.py Outdated Show resolved Hide resolved
ribs/archives/_sliding_boundaries_archive.py Show resolved Hide resolved
tests/emitters/cma_me_emitter_test.py Outdated Show resolved Hide resolved
tests/optimizers/optimizer_test.py Outdated Show resolved Hide resolved
Copy link
Member

@btjanaka btjanaka left a comment

Choose a reason for hiding this comment

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

minor

HISTORY.md Outdated Show resolved Hide resolved
ribs/archives/_archive_base.py Outdated Show resolved Hide resolved
ribs/visualize.py Outdated Show resolved Hide resolved
tests/visualize_test.py Outdated Show resolved Hide resolved
@btjanaka btjanaka changed the title Remove init Remove initialize method from archives Jun 9, 2022
Elle Szabo added 2 commits June 8, 2022 19:03
ribs/archives/__init__.py Show resolved Hide resolved
@eszabo12 eszabo12 merged commit 044eb2e into master Jun 9, 2022
@eszabo12 eszabo12 deleted the remove_init branch June 9, 2022 02:29
@btjanaka btjanaka added the API label Jun 22, 2022
@btjanaka btjanaka mentioned this pull request Jun 23, 2022
6 tasks
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 this pull request may close these issues.

None yet

2 participants