test for classic controllers, dc motor, dcExternallyExcited motor #261
Conversation
…ttps://github.com/upb-lea/gym-electric-motor into add-further-automated-testing-of-existing-features
| #parameters for dc motor | ||
| test_dcmotor_parameter = {"r_a": 16e-2, "r_e": 16e-1, "l_a": 19e-5, "l_e_prime": 1.7e-2, "l_e": 5.4e-1, "j_rotor": 0.025} | ||
| test_dcmotor_nominal_values = dict(omega=350, torque=16.5, i=100, i_a=96, i_e=87, u=65, u_a=65, u_e=61) | ||
| test_dcmotor_limits = dict(omega=350, torque=38.0, i=210, i_a=210, i_e=215, u=60, u_a=60, u_e=65) |
There was a problem hiding this comment.
I'm not sure if it's good to set the limits lower then the nominal values for the voltages. (parameters starting with an u)
There was a problem hiding this comment.
@bhk11 Sure, I will set try to update all the limits values to be higher than nominal values
| @@ -28,25 +28,25 @@ def __init__( | |||
| torque_control="interpolate", | |||
| **controller_kwargs, | |||
| ): | |||
| t32 = environment.physical_system.electrical_motor.t_32 | |||
| q = environment.physical_system.electrical_motor.q | |||
| t32 = environment.unwrapped.physical_system.electrical_motor.t_32 | |||
There was a problem hiding this comment.
Nice work, that the test run trough successful!
Personally, I would prefer to unify the code and use get_wrapper_attr('physical_system') instead of unwrapped.physical_system .
In my opinion this getter should also work for the action_space, state_names and so on.
There was a problem hiding this comment.
Hi,
This proposed change has been made and will be pushed with the commit message ""change to get_wrapper_attr()""
There was a problem hiding this comment.
Good test structure already! Would be nice to test a few more combinations of test motors with different parameters and different states + state transitions. E.g., initializing a motor in different initial states, applying a voltage and asserting the correct resulting state. Repeat this for different motor parameters.
For readibility these different parameters shouldn't each have their own fixture, though. Either create an array of Motors in a single fixture or initialize the motors inside the test case. Thanks for the effort :)
| assert concreteDCMotor.motor_parameter == test_dcmotor_parameter | ||
| assert concreteDCMotor.nominal_values == test_dcmotor_nominal_values | ||
| assert concreteDCMotor.limits == test_dcmotor_limits | ||
| assert concreteDCMotor._initial_states == {"i_a": 10.0, "i_e": 15.0} |
There was a problem hiding this comment.
test_dcmotor_default_initializer["states"] would be better instead of {"i_a": 10.0, "i_e": 15.0}
There was a problem hiding this comment.
Sure, Will make the change
|
|
||
| extex_state_space = Box(low=-1, high=1, shape=(7,), dtype=np.float64) | ||
| assert concreteDCMotor._initial_states == default_initial_state | ||
| concreteDCMotor._initial_states = {'i_a': 100.0, 'i_e': 20.0} |
There was a problem hiding this comment.
concreteDCMotor._initial_states = new_initial_state
|
|
||
| assert concreteDCMotor._initial_states == new_initial_state | ||
| assert np.array_equal(concreteDCMotor.reset(extex_state_space,extex_state_positions),default_Initial_state_array) | ||
| assert concreteDCMotor._initial_states == default_initial_state |
There was a problem hiding this comment.
Could you also test the behavior when concreteDCMotor._initializer["states"] is overwritten? In that case we assume the new initial values will not be overwritten on reset.
There was a problem hiding this comment.
This is what I have already tried to do with "test_ConcreteDCMotor_reset", first the motor is initialized with a state different from the one in DCMotor.py and then the state is again changed with the "new_initial_state". On reset the intial_State is changed to default_initial_state which is different from the value in the DCMotor.py. I hope this is what you expect. Thank you!
Could you please review these changes