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

Set config properties with __setitem__ and type dispatching #148

Closed
wants to merge 1 commit into from

Conversation

pierrelux
Copy link
Contributor

The proposed change adds the ability to configure ALE using __setitem__.

The typed nature of C++ has probably motivated the need to write dedicated "setFloat/setInt/setString/setBool" functions. However, a unified access to these methods can be provided in python using dynamic type dispatching. The proposed changes simply checks for the type of the value argument and calls the corresponding setter in the C API.

Here's an example of the proposed change in action:

from ale_python_interface import ale_python_interface as ALE

ale = ALE.ALEInterface()

ale[b'random_seed'] = 2468
ale[b'frame_skip'] = 5
ale[b'color_averaging'] = True

print(ale.getInt(b'random_seed'))

which in this case just prints 2468. Note that the corresponding __getitem__ could also be provided. However, it is difficult to implement type dispatching since the type couldn't be inferred from the value (which is the return type, and not an argument anymore). Ideally, Settings.hxx should expose all of the available configuration keys and their associated types. The python interface could then read the header and perform the appropriate type dispatching.

The proposed changes have no effect on the existing interface other than adding a new function. All of the existing functions remain untouched and there should be no backwards compatibility issues.

@mgbellemare
Copy link
Contributor

@pierrelux , looks like a nice improvement. Part of the aim of using separate methods for each type was to provide an assertion when attempting to change an invalid setting. This still works here, so all good. One thing that might cause trouble is methods which take in float values but may be called with integer arguments. E.g.

ale[b'repeat_action_probability'] = 1

This will trigger the assert. Can you add a comment to document this case? Otherwise LGTM.

@JesseFarebro
Copy link
Collaborator

Since this PR is no longer compatible with the new Python interface I think it can be closed. I created an issue (#367) to track this feature as I think this can be a valuable addition to the Python interface.

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.

3 participants