Conversation
- fixed bugs and refactored encoder and decoder related methods.
…ed action instances. the encoder\decoder method are outside and testable fucntions (i.e. see esam_learning_tests).
argaman-aloni
added
the
code-maintenance
sam_learning/learners/esam.py
Outdated
| proxy_action_call: grounded action call of the learned domain | ||
| new_proxy: the lifted instance of the grounded action call | ||
| proxy_data: data of the construction of the proxy actions | ||
| action_name: the original action's name. |
There was a problem hiding this comment.
Isn't this also part of the proxy action data object? and if not, why?
There was a problem hiding this comment.
they are not a part of the data.
"action_name" is the original action name and is not needed except for the encoder decoder methods since it appears in the outer scope of the call.
it is taken as an argument in order to bind those parameter when called inside a loop to the lambda arguments (though they can be explicitly bounded if called as an argument inside the lambda. i prefered this option).
the "new_proxy" can be saved inside the data
"proxy_action_call" is different at every call for for the method so it is an unbounded argument that is bounded only when encoder\decoder is needed.
| """ | ||
| original_lifted_action = self.partial_domain.actions[action_name] if action_name in self.partial_domain.actions\ | ||
| else self.popped_actions_archive[action_name] | ||
| original_signature = list(original_lifted_action.signature.keys()) |
There was a problem hiding this comment.
inline the usage (only single usage, so no need for additional variable)
| original_lifted_action = self.partial_domain.actions[action_name] if action_name in self.partial_domain.actions\ | ||
| else self.popped_actions_archive[action_name] | ||
| original_signature = list(original_lifted_action.signature.keys()) | ||
| proxy_param_list = list(new_proxy.signature.keys()) |
sam_learning/learners/esam.py
Outdated
There was a problem hiding this comment.
inline this as well, just use the .items() where you need it.
|
|
||
| new_obj_list = [original_action_map[param] for param in original_signature] | ||
| return ActionCall(action_name, new_obj_list) | ||
| def encoder_method(self, original_action_call: ActionCall, proxy_data: ProxyActionData, new_proxy: LearnerAction, |
| raise NotSafeActionError(name=f"{new_proxy.name}", | ||
| reason="proxy action parameter mapping" | ||
| "does not align with the action call", | ||
| solution_type=EquationSolutionType.no_solution_found) |
There was a problem hiding this comment.
This error type is not well fitted for what you want to do here... And notice that if you raise an error and not catch it later the entire program will stop. Is this what you want?
There was a problem hiding this comment.
no. what error is appropriate for this case?
tests/esam_learning_test.py
Outdated
There was a problem hiding this comment.
don't forget the type annotation.
| ), | ||
| ] | ||
| ) | ||
| def test_decoder_full_example(rovers_esam_learner, rovers_esam_observation, proxy_action_call: ActionCall, |
There was a problem hiding this comment.
I personally don't like the parameterization option if the outcome of the test indicates different behaviour since the behaviour is not explained (usually the test name gives indication on what you expect...)
I would like you to explain better when you test here, this does not mean you have to do much, just split this to tests and give indicative names to the test cases. OR you can keep the current format with the parameterization but give full indicative name to the test case, AND improve the documentation :)
There was a problem hiding this comment.
okay i will' iin the meantime ill the changes and handle it later on today
| for param_name, obj in original_param_mapping.items()} | ||
| for param_name, obj in original_param_mapping.items(): | ||
| if grounded_proxy_parameters[proxy_data.signature[param_name]] != obj: | ||
| raise NotSafeActionError(name=f"{new_proxy.name}", |
There was a problem hiding this comment.
Be careful, think about who will call this method and whether the error is being cought.
2 participants
implemented encoder-decoder methods for easmm and added tests to esam test file