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

Type annotations #70

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MichaelJFishman
Copy link
Contributor

@MichaelJFishman MichaelJFishman commented Dec 30, 2022

Description

Type annotations

Changed State to a typed NamedTuple

Split off from #67

Tests

Once #68 is merged, I'll rebase this branch and run the tests.
Ran unit tests.

@MichaelJFishman
Copy link
Contributor Author

Some thoughts:

I think State should subclass gym.core.ObsType. There should possibly be other gym subclassing. Once the unittest PR is merged in, I may try that subclassing and see if it works. I'm not familiar with having a NamedTuple class that also subclasses another class, so I want to have the tests setup before making that change.

Some of the classes (eg Exists) could be cleaned up by using dataclasses, which can autogenerate __hash__, __eq__, __repr__, etc.

We may want to make Predicate, ForAll, etc. share a parent class, since all those seem like they'd be used in similar situations, eg as a goal.

I generally think that having stricter types for classes makes code easier to reason about, and enables more powerful static analysis. Currently PDDLDomain is initialized with most of its attributes as None. It would be nice if we could make PDDLDomain NEVER have None for most of these attributes, so that if you have a PDDLDomain object, you know for sure you can use it's actions, etc. I don't really know a good way to accomplish this without giving up the flexibility to parse a domain piecemeal. We could just make PDDLDomainParser prepare the attributes, then bundle them into a PDDLDomain that immediately has all the needed attributes. But I'm not confident this is the right move. Since I'm unclear about this part, I'm leaving that out of this PR for now.

Changed State to a typed NamedTuple
@MichaelJFishman
Copy link
Contributor Author

This branch passed the existing unit tests.

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.

1 participant