Closed rimvydasrub closed 6 months ago
No rush. Better be certain than buggy 🐛
On Wed, 29 May 2024, 17:55 Peter David Fagan, @.***> wrote:
@.**** approved this pull request.
LGTM will merge after running quick test as I haven't set up automated unit testing yet.
— Reply to this email directly, view it on GitHub https://github.com/peterdavidfagan/mujoco_robot_environments/pull/3#pullrequestreview-2085949581, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADS5XGXCBLL65UEYNTZ7XGTZEYB7JAVCNFSM6AAAAABIPHUJQGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOBVHE2DSNJYGE . You are receiving this because you authored the thread.Message ID: @.*** .com>
This change requires refactoring a number of existing functions for sorting blocks etc. I will perform this refactor later today.
Are you able to continue progressing locally while I work on this refactor? As in pip install your dev version of the package (this pull request) while I refactor and run a new release for PyPI?
Yes i have more to do besides this. Including adding support for evaluation and pragmatics.
Yet should be done with all this by the end of today.
On Thu, 30 May 2024, 08:55 Peter David Fagan, @.***> wrote:
This change requires refactoring a number of existing functions for sorting blocks etc. I will perform this refactor later today.
Are you able to continue progressing locally while I work on this refactor? As in pip install your dev version of the package while I refactor and run a new release for PyPI?
— Reply to this email directly, view it on GitHub https://github.com/peterdavidfagan/mujoco_robot_environments/pull/3#issuecomment-2138902287, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADS5XGQPVQEJILSIIFCDTDLZE3LOXAVCNFSM6AAAAABIPHUJQGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZYHEYDEMRYG4 . You are receiving this because you authored the thread.Message ID: @.***>
Understood, thanks for communicating the above, I will plan my dev work accordingly.
Yes. What you can have is dataclass atributes with default fields: colour, shape, texture which is then can be accessed in a robust way in various instances for tasks
I can do this later tomorrow.
On Fri, 31 May 2024, 19:53 Peter David Fagan, @.***> wrote:
@.**** requested changes on this pull request.
@rimvydasrub https://github.com/rimvydasrub one issue I am encountering with the current formulation is that the labels list doesn't explicitly provide access to specific attributes for instance colour. Would it be possible to reformulate with a custom data class and/or another datastructure such that this is possible.
— Reply to this email directly, view it on GitHub https://github.com/peterdavidfagan/mujoco_robot_environments/pull/3#pullrequestreview-2091524388, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADS5XGXSFABR33CTCGRM33TZFDBL7AVCNFSM6AAAAABIPHUJQGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOJRGUZDIMZYHA . You are receiving this because you were mentioned.Message ID: @.*** .com>
Thanks @rimvydasrub feel free to ping me one done and I will retest and merge. Right now for the transporter network data collection and other utilities distinguishing colour from shape for instance is important.
@peterdavidfagan I have added type annotations for the labels, by creating the dataclass below:
@dataclass
class PropsLabels:
"""Container for prop labels."""
data: dict = field(default_factory=dict)
default_values: dict = field(default_factory=lambda: {"texture": "plain"})
def __post_init__(self):
# Set default values
for key, value in self.default_values.items():
setattr(self, key, value)
# Override with values from the input dictionary
for key, value in self.data.items():
setattr(self, key, value)
def __str__(self):
attrs = ', '.join(f"{key}='{value}'" for key, value in self.__dict__.items() if key not in {'data', 'default_values'})
return f"PropsLabels({attrs})"
def __repr__(self):
return self.__str__()
using int when creating a prop you can access label information like this:
from mujoco_robot_environments.tasks.rearrangement import RearrangementEnv
env = RearrangementEnv(viewer=True)
env.reset()
prop_labels = env.props[0].labels # labels of the first prop
print(prop.labels.colour) # get colour ; Note this would yield an error colour label is not provided e.g. now for an apple
print(prop.labels.texture) # returns plain texture by default
...
I tried to add this functionality in colour spliiter too. Please review and give feedback if the functionality is now right
With this PR I am moving the name it to be
prop{idx}
and then having a label field to generate symbols for props info.