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

Add params and rng argument to all FuncEnv member functions #900

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

Kallinteris-Andreas
Copy link
Collaborator

@Kallinteris-Andreas Kallinteris-Andreas commented Feb 1, 2024

Description

Fixes #851, #861

Type of change

  • Documentation only change (no code changed)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@Kallinteris-Andreas Kallinteris-Andreas marked this pull request as ready for review February 1, 2024 09:01
Copy link
Member

@RedTachyon RedTachyon left a comment

Choose a reason for hiding this comment

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

At a glance seems alright, except it would be nice if the rng argument was also optional. Can you check if that works well, and if so, add tests for all variants of calling the functions? (implicit None, explicit None, explicit non-trivial arguments)

return state[1] > 0


def test_api():
env = BasicTestEnv()
state = env.initial(None)
obs = env.observation(state)
obs = env.observation(state, None)
Copy link
Member

Choose a reason for hiding this comment

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

Can we make the rng argument optional the same way as params?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How should rng default be handled?
If an environment's needs transition member function requires rng, what should be done?

In the case of parameters, there is clearly a default

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Kallinteris-Andreas
Copy link
Collaborator Author

Kallinteris-Andreas commented Mar 16, 2024

@RedTachyon check the above comment Thanks

Kallinteris-Andreas and others added 5 commits April 5, 2024 21:12
Co-authored-by: Mark Towers <mark.m.towers@gmail.com>
Co-authored-by: Pratik Ingle <prin@itu.dk>
Co-authored-by: Jose Antonio Martin H <ja.martin.h@repsol.com>
Co-authored-by: Oli <ollihaus@t-online.de>
Co-authored-by: Jared Swift <j.w.swift@outlook.com>
Co-authored-by: Tim Schneider <mail@tim-schneider.me>
Co-authored-by: Tim Schneider <tim@robot-learning.de>
Co-authored-by: Tim Schneider <tim.schneider94@t-online.de>
Co-authored-by: Manuel Goulão <msilvagoulao@gmail.com>
Co-authored-by: Michael Panchenko <35432522+MischaPanch@users.noreply.github.com>
Co-authored-by: TobiasKallehauge <tkal@es.aau.dk>
Co-authored-by: Ariel Kwiatkowski <ariel.j.kwiatkowski@gmail.com>
Co-authored-by: James Mochizuki-Freeman <jameymmf@gmail.com>
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.

[Question] FuncEnv why is the key not argument for all member functions
2 participants