search

google-deepmind / dm_env

A Python interface for reinforcement learning environments
Apache License 2.0
342 stars 43 forks source link

Consider renaming array classes #5

Closed cool-RR closed 3 years ago

cool-RR commented 3 years ago

Take this more as feedback, because I'm guessing the names are too deeply ingrained now.

I had to do a double-take to understand that a BoundedArray object is not a bounded array, but a spec for a bounded array. This is counter intuitive. It also applies to Array, DiscreteArray, etc. It would have be nicer in my opinion if these were called BoundedArraySpec, ArraySpec, DiscreteArraySpec, etc.

alimuldal commented 3 years ago

The intent is for you to import the whole specs module and access these classes via the module namespace - that way the module name disambiguates that these are specs, e.g. specs.BoundedArray is clearly a spec for a bounded array. I can see that these names might be confusing if you are in the habit of importing classes individually, but this has not really been an issue for us since we don't generally use this style of import (it's forbidden by Google's style guide).

In fact, prior to open-sourcing these classes did have the Spec suffix you're suggesting, but we removed it because we felt it was redundant and made the class names excessively long to type.

cool-RR commented 3 years ago

I understand, but that's looking from the perspective of dm_env instead of the perspective of someone doing research who's using dozens of Python libraries,

Here's my experience. I was learning to use Acme, and I tried the official tutorial. I was running the code cell-by-cell, until I got to this part:

At this point I didn't even know that dm_env existed. I was just wondering, what's in these BoundedArray objects? I tried exploring them, and was surprised that I can't do array[0] to find the value. I read the documentation and saw that they're just specs for an array rather than actual arrays. Now I'll remember that, but I do think it adds confusion.

alimuldal commented 3 years ago

I see. In this example I'd have hoped that the name environment_spec, plus the string representations, would have given enough of a clue. As you touched on, the dm_env interface is very widely used, so changing these classnames at this point would be a bit disruptive. Also, as I mentioned, we originally dropped the Spec suffix in response to user feedback, so I would anticipate resistance to reinstating it.