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

BIG PR: Simplification and homegenisation of environments, addition of tests, other clean up changes #101

Merged
merged 118 commits into from Mar 31, 2023

Conversation

alexhernandezgarcia
Copy link
Owner

@alexhernandezgarcia alexhernandezgarcia commented Mar 26, 2023

  • python -m pytest tests/gflownet/envs/
  • black .
  • isort .

It's quite a massive PR and it's hard to mention all changes, but here is a non-exhaustive list of things:

  • I have simplified the init method of the environments, such that everything that is common to all environments is now in the init of the parent class (base.py) and super() is called at the end of the init.
  • Similar thing for the arguments: the arguments of an environment should only be those that are specific to the environment. Everything else goes in the kwargs.
  • And methods that were common or very similar for all environments have been unified into the parent class. A good example is reset(). Now it's common for all environments - I think - therefore there is no need to re-implement it.
  • All environments now define the self.source state in the init, and this is used by reset() (in the parent class) to initialise the first self.state.
  • The end-of-sequence (self.eos) in the environments is now an action with the format of other other actions and not an integer or whatever. This is more consistent in my opinion.
  • There is a new method in the parent env class, actions2indices() that is used by the flowmatch loss.
  • All proxies can implement a setup() method that receives the environment as argument and sets whatever variables are necessary, like n_dim. setup() is called in the init of the parent env class.
  • In general, the proxies have also been cleaned up, and the uniform proxy is now working, which is useful for debugging.
  • The buffer class is now in a separate script in utils/buffer.py
  • In the grid and the (discrete) torus, extended actions are now handled via the variables max_increment and max_dim_per_action, which specify how many positions (e.g. cells in the grid or discrete angles in the torus) can be done in one action and how many dimensions can be updated in one single action.
  • I have re-arranged the order of methods in the environment class.
  • I have extended and clarified the docstrings.
  • I have added more mypy types - now most methods should have it. If missing, we should add them.
  • torus_rounds.py was obsolete and has been removed.#
  • I have added a few more assert statements for extra security.
  • There are many new tests (although there should be more!) and since many are common to all environments, these are grouped in tests/gflownet/env/common.py
  • self.get_actions_space() is now self.get_action_space(); get_max_traj_len is now get_max_traj_length; self.d_action_space is now self.action_space_dim.

Some things remain to be done:

  • The aptamers (aptamers.py) environment has not been cleaned up and updated because I have the more ambitious plan of creating a Sequence parent class from which the aptamers will be inherited.
  • The plane (plane.py) environment has not been cleaned up and updated because it is not working well yet.
  • We need more tests, especially for the continuous environments, and for the GFlowNet agent itself.
  • Docstrings need to keep improving.

⚠️ I am not requesting careful reviews of the PR since it is huge, but reviews of things here and there and comments are very welcome. After merging this into continuous, my plan is to merge continuous into main (without further review, obviously) and continue improving things from there.

@nikita-0209
Copy link
Collaborator

Regarding the aptamers bullet point, I have created a Sequence parent class from which child classes Aptamers and AMP are derived in the cont_mf branch. I will merge this branch into cont-mf and then create a PR with those changes.

@alexhernandezgarcia
Copy link
Owner Author

Regarding the aptamers bullet point, I have created a Sequence parent class from which child classes Aptamers and AMP are derived in the cont_mf branch. I will merge this branch into cont-mf and then create a PR with those changes.

Oh I didn't know you had done this already - that's great, thanks!

gflownet/gflownet.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@michalkoziarski michalkoziarski left a comment

Choose a reason for hiding this comment

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

Doesn't have to be a part of this PR, but I think it's time to group envs related to crystal generation into a single directory.

gflownet/envs/base.py Show resolved Hide resolved
gflownet/envs/base.py Outdated Show resolved Hide resolved
gflownet/envs/base.py Outdated Show resolved Hide resolved
gflownet/envs/base.py Show resolved Hide resolved
parents_a: Optional[List] = None,
) -> List:
"""
Returns a list of length the action space with values:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could use argument description in docstring (in particular for parent_a).

gflownet/envs/spacegroup.py Outdated Show resolved Hide resolved
"""
Resets the environment.
"""
self.state = []
self.state = self.source.copy()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to have it as a method, smth like self.get_source(reset=True) to accommodate scenario when we start from random state

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, to be done when needed 👍

alexhernandezgarcia and others added 7 commits March 30, 2023 20:50
Co-authored-by: Alexandra <alexandra.volokhova@mila.quebec>
Co-authored-by: Alexandra <alexandra.volokhova@mila.quebec>
Co-authored-by: Michał Koziarski <michal.koziarski@gmail.com>
Co-authored-by: Michał Koziarski <michal.koziarski@gmail.com>
@alexhernandezgarcia alexhernandezgarcia merged commit 3e18a27 into continuous Mar 31, 2023
@josephdviviano josephdviviano deleted the tests-and-simplify-envs branch January 31, 2024 21:44
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

4 participants