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

Updated Environment to implement 1D objects for 1D environments. #98

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

colleenjg
Copy link
Contributor

Implemented objects for 1D environments (adding them to the environment and including them in the plots).
Added checks for parameters passed during environment initialization that do not apply to 1D environments (holes, walls).

@@ -71,7 +71,7 @@ class Environment:
"boundary": None, # coordinates [[x0,y0],[x1,y1],...] of the corners of a 2D polygon bounding the Env (if None, Env defaults to rectangular). Corners must be ordered clockwise or anticlockwise, and the polygon must be a 'simple polygon' (no holes, doesn't self-intersect).
"walls": [], # a list of loose walls within the environment. Each wall in the list can be defined by it's start and end coords [[x0,y0],[x1,y1]]. You can also manually add walls after init using Env.add_wall() (preferred).
"holes": [], # coordinates [[[x0,y0],[x1,y1],...],...] of corners of any holes inside the Env. These must be entirely inside the environment and not intersect one another. Corners must be ordered clockwise or anticlockwise. holes has 1-dimension more than boundary since there can be multiple holes
"objects": [], #a list of objects within the environment. Each object is defined by it's position [[x0,y0],[x1,y1],...]. By default all objects are type 0, alternatively you can manually add objects after init using Env.add_object(object, type) (preferred).
"objects": [], # a list of objects within the environment. Each object is defined by it's position [[x0,y0],[x1,y1],...]. By default all objects are type 0, alternatively you can manually add objects after init using Env.add_object(object, type) (preferred).
Copy link
Collaborator

Choose a reason for hiding this comment

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

the current [[x0,y0],...] implies 2D, shall we rewrite this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point!

Copy link
Contributor Author

@colleenjg colleenjg Dec 6, 2023

Choose a reason for hiding this comment

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

What do you think of this: Each object is defined by its position [[x0,y0],[x1,y1],...] for 2D environments and [[x0],[x1],...] for 1D environments.

self.Agents: list[Agent] = [] # each new Agent will append itself to this list
self.agents_dict = (
{}
) # this is a dictionary which allows you to lookup a agent by name
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume you're using a code formatter. can this go back to being 1 line or is

dict = (
{}
}

better for some reason I'm not aware?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh dear - my autoformatter ran on this file. I didn't mean for it to. I'll update the PR to remove the autoformatting changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

to be honest that would be great for now but as I said below...we should probably adopt a common style :)))

f"You have passed {feature_for_2D_only} into a 1D environment. "
"This is ignored."
)
setattr(self, feature_for_2D_only, list())
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for doing this!

autosave=None,
plot_objects=True,
**kwargs,
):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, happy for plot_objects to be an explicit kwarg but if so don't forget to add it to the doc string!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add!

Copy link
Collaborator

@TomGeorge1234 TomGeorge1234 left a comment

Choose a reason for hiding this comment

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

nice, thanks for doing this. I left a few specific comments on various lines, reply/solve them and we can merge! Some higher level things:

  • We should probably think about committing to a code style. I notice your code profiler has made lots of minor changes which are probably all good but like...we should make this uniform across the code base.
  • Check out RatInABox2.0 - Roadmap #84 last two comments. In the long run I'd like to make all environmental features (walls, boundaries, objects, etc.) their own classes which would extract a lot of these complexities out of the Environment class into a distinct EnvironmentFeatures classes. I think it would simplify the code a lot and make adding crazy wild things like teleports etc. much more straightforward. Feel free to throw in your two-cents. I'm saying this because in the long run these changes may be redundant but for now good to keep up object support inside the environment class.

@colleenjg
Copy link
Contributor Author

Thanks for your thorough checks, Tom!

I've fixed the PR:

  • removed the accidental formatting changes! (I definitely agree: no point in making ad hoc changes to the formatting style unless there's an agreed upon plan, so I always aim to match the original style as much as possible)
  • updated the objects definition to describe objects format in 1D and 2D
  • Added plot_objects to the doc string for plot_environment().

@TomGeorge1234
Copy link
Collaborator

woohoo! excellent stuff, merged and grateful as always for the excellent work :))

@TomGeorge1234 TomGeorge1234 merged commit 20f6d2e into RatInABox-Lab:main Dec 6, 2023
3 checks passed
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.

2 participants