UFRN-URNAI / urnai-tools

A modular Deep Reinforcement Learning library that supports multiple environments, made with Python 3.6.
Apache License 2.0
5 stars 8 forks source link

Resolves #83 #85

Closed RickFqt closed 3 months ago

RickFqt commented 5 months ago

Resolves #83.

RickFqt commented 5 months ago

All the code has comments of the type TODO or indicating some improvement. This is the time to resolve these issues and remove these comments.

To be honest, I put these TODO there to check on them later.

For the first one on the start() function, I thought it would be a good idea to let the user define the parameters used in the definition of agent_interface_format. In that case, the user should provide the parameters from the init function, right?

The second TODO was a reafactoring issue. In the original code, those lines were like this:

setattr(obs, 'step_mul', self.step_mul)
setattr(obs, 'map_size', self.env_instance._interface_formats[0]._raw_resolution)

But when I ran the linter code, it discouraged the use of setattr, and that it would be better to do a simple assignment. After that, I still wasn't able to test if these attributes of "obs" really exist, and I am still struggling on how to check that out.

alvarofpp commented 5 months ago

@RickFqt

In that case, the user should provide the parameters from the init function, right?

It can be in __init__ or even in start.

RickFqt commented 4 months ago

It would be good to use type hint in the functions.

Is it necessary even in functions with default parameters?

RickFqt commented 4 months ago

It would be good to use type hint in the functions.

I've been trying to find the type of the action parameter from the step() method, but I'm having some difficulty to do so. In this method, the pysc2 own method step() is called with the same action parameter, so I started looking at its documentation here. At first, I thought that an action would be an instance of FunctionCall, since its what is returned from the RandomAgent implementation from pysc2. However, when trying to test this method, I wasn't able to instantiate a FunctionCall properly, and kept getting an error. This is how I tried to test the method:

from pysc2.lib import actions

from urnai.sc2.environments.sc2environment import SC2Env

env = SC2Env()
obs = env.reset()

# Trying to create a function call with ID 2 (select_point)
# and ARGS = [[0], [23, 38]]
env.step(actions.FunctionCall(2, [[0], [23, 38]]))

That said, I'm not sure if I'm creating a FunctionCall object incorrectly, or if it's not the right parameter type for step. I also tried to look for the previous implementation of urnai, but got confused as well.

alvarofpp commented 4 months ago

Is it necessary even in functions with default parameters?

Yes, type hint also helps IDEs when it comes to displaying function signatures.

alvarofpp commented 4 months ago

It would be good to use type hint in the functions.

I've been trying to find the type of the action parameter from the step() method, but I'm having some difficulty to do so. In this method, the pysc2 own method step() is called with the same action parameter, so I started looking at its documentation here. At first, I thought that an action would be an instance of FunctionCall, since its what is returned from the RandomAgent implementation from pysc2. However, when trying to test this method, I wasn't able to instantiate a FunctionCall properly, and kept getting an error. This is how I tried to test the method:

from pysc2.lib import actions

from urnai.sc2.environments.sc2environment import SC2Env

env = SC2Env()
obs = env.reset()

# Trying to create a function call with ID 2 (select_point)
# and ARGS = [[0], [23, 38]]
env.step(actions.FunctionCall(2, [[0], [23, 38]]))

That said, I'm not sure if I'm creating a FunctionCall object incorrectly, or if it's not the right parameter type for step. I also tried to look for the previous implementation of urnai, but got confused as well.

For functions, you can use the type hint typing.Callable.

RickFqt commented 4 months ago

For functions, you can use the type hint typing.Callable.

I've searched a little bit and it seems like importing typing.Callable is deprecated, as well as some other modules from typing (see here). Is that right? If so, should I change the other things imported from typing like typing.List or typing.Tuple?

alvarofpp commented 4 months ago

For functions, you can use the type hint typing.Callable.

I've searched a little bit and it seems like importing typing.Callable is deprecated, as well as some other modules from typing (see here). Is that right? If so, should I change the other things imported from typing like typing.List or typing.Tuple?

That's right, I didn't know that. We will adopt the imports suggested by PEP from now on.

RickFqt commented 4 months ago

All that remains is to remove the comments and insert the unit tests.

About the unit tests, I have some questions:

alvarofpp commented 4 months ago
  • Do we have to test the functionality of other classes that are used inside the class? For example, here in our SC2Env class, we have an attribute called env_instance, which is initialized with an instance of another class (sc2_env.SC2Env). When a method of SC2Env calls methods from env_instance, do we have to check the behavior of attributes that are inside env_instance?

In the case of unit tests, we don't need to test the classes of third-party packages or the classes used by the class we're testing. The recommended thing is to create mocks for these other classes and simulate the expected behavior.

  • When testing the method step, I was using the previous implementation of urnai's action wrapper to create instances of actions, and then passed them as a parameter to step to test the functionality of some actions (like build refinery, for example). Is there a way of not using this and still simulate the functionality of the actions?

I recommend that you only use the old code as a reference and not as a test. For the tests, try to stick to the code we currently have in v2.

You can and should use mocks to simulate the expected behavior (as you did with the fake classes in another issue).