allenai / ScienceWorld

ScienceWorld is a text-based virtual environment centered around accomplishing tasks from the standardized elementary science curriculum.
https://sciworld.apps.allenai.org/
Apache License 2.0
199 stars 24 forks source link

Python API does not follow PEP 8 #54

Closed AndKaminer closed 8 months ago

AndKaminer commented 8 months ago

Hi there, I'm newly interested in contributing to this project, and I noticed that the python API doesn't follow python naming convention. I figured (and Dr. Cote suggested) that it would be a good first issue for me to change that API so that it does follow convention. Please let me know if that breaks anything. Otherwise, just let me know, and I'll make a PR.

By the way, is there any specified format/information required for pull requests?

Thanks!

PeterAJansen commented 8 months ago

Hi @AndKaminer, thanks for your note. Contributions are very welcome!

The API right now uses camelCase instead of snake_case for function names, and changing this would likely introduce breaking changes (i.e. it would require anyone upgrading ScienceWorld to change their agent code to snake_case). I'm not sure that would yield a great benefit/cost ratio, but of course if others feel it would, then please feel free to submit a PR.

For other contribution ideas, now that some agents (e.g. SwiftSage, CLIN, etc) are starting to have substantially increased performance over the baseline agent, I wonder what it would look like to add in a few more tasks that are harder, or that have more ablations. For example, the change-of-state tasks sometimes have device ablations (e.g. the stove might suddenly be broken in some variations), requiring a modification to the agent's solution strategy (e.g. using a different heat source -- from the blast furnace in the foundry, to building a campfire outside). When we designed the SW tasks, the solutions were originally intended to be a little less formulaic, but the agents of the time (e.g. DRRN, T5 Behavior Cloning Baseline) were so terrible that it seemed like making them more regular would help agents begin to learn them. Now that agents are doing well, I wonder if there are ways of extending the environment's utility (in terms of making tasks harder/less formulaic) that are low-hanging fruit.

AndKaminer commented 8 months ago

Those sounds like great ideas. I will see what I can do about them. I assume more testing is also never a bad thing?

MarcCote commented 8 months ago

For context, I did suggest @AndKaminer the idea of refactoring the code to be compliant with PEP8. I believe we can do a slow transition and keep the critical methods that use camelCase naming convention has a legacy (but raise a deprecation warning).

MarcCote commented 8 months ago

Here are some things to be done to be compliant with PEP8 (and more).

AndKaminer commented 8 months ago

Here are some things to be done to be compliant with PEP8 (and more).

* [X]  Use snake case

* [X]  Deprecate old method that uses CamelCase

* [ ]  Add Github Action to run tests and flake8 on the code

* [ ]  Use flake8 to fix PEP8 errors

* [ ]  Fix other scripts (and code examples) that relies on the deprecated methods.

I'll put up another PR for the remaining.

AndKaminer commented 8 months ago

Oops I didn't mean to close

AndKaminer commented 8 months ago

Here are some things to be done to be compliant with PEP8 (and more).

* [x]  Use snake case

* [x]  Deprecate old method that uses CamelCase

* [x]  Add Github Action to run tests and flake8 on the code

* [x]  Use flake8 to fix PEP8 errors

* [X]  Fix other scripts (and code examples) that relies on the deprecated methods.