Repository navigation
Added testing for saving - #78
Conversation
Created simple tests for ModularAgent to test every supposedly legal agent configuration. Some are currently broken.
Excessive print statements were making it hard to read test output.
Fixed up some tests so that the majority of the configurations now pass the basic test.
Needed to fix a relative pathing issue with loading testing files.
It looked like specifictogeneral is universally broken for the fo_planner right now so I have removed it from the test profile in the interest of commiting a full pass test suite to check coverage reports.
Changes to re-enable saving had not be properly migrated after adding tests for saving. As far as I can tell they seem to work though may still have issues with different kinds of operations in learned skills.
eharpste
left a comment
eharpste
left a comment
There was a problem hiding this comment.
Took a pass through the new test_save. I added several comments for removing redundant tests and generally cleaning up the file. I'm already actively working on making the needed changes on my copy of the branch so don't worry about following up.
| #ModularAgent()-- then request, train, give answers when dont know what to do | ||
| #when guesses and correct -- look same as when agent asks for hint and gets hint back-- reward should be 1 | ||
| #needs value + cntentEditable | ||
| def test_FO_ModularAgent(): |
There was a problem hiding this comment.
It looks like this only tests a single configuration of an FO_Planner based agent rather than all configurations like the numbert one. Even if some of these configurations fail we'll want to know. Ideally this test case would be a parallel of the ModularAgent one.
|
|
||
| #check that copy is the same | ||
| dc = copy.deepcopy(resp) | ||
| assert(dc == resp) |
There was a problem hiding this comment.
This check seems like an unnecessary test since its just confirming the behavior of the external deepcopy library working on a built in dictionary type.
| assert(agent.__dict__['epsilon'] == 100) | ||
| agent_stream = dill.dumps(agent) | ||
| agent_deserialized = dill.loads(agent_stream) | ||
| assert(agent_deserialized.__dict__['epsilon'] == 100) |
There was a problem hiding this comment.
Not sure we need these final checks, since they are mainly checking against a possible language feature that doesn't exist.
| for common in to_find_common: | ||
| assert(agent1dict[common] == agent2dict[common]) | ||
|
|
||
| #checking rhs_by_how |
There was a problem hiding this comment.
Would be great to remove a lot of this dead code.
| resp, ans = train_A_req_A(agent) | ||
| assert resp['selection'] == ans['selection'] | ||
| assert resp['action'] == ans['action'] | ||
| assert str(int(resp['inputs']['value'])) == ans['inputs']['value'] |
There was a problem hiding this comment.
I could see removing the asserts above as they duplicate the tests done in the configuration test. Not necessarily a bad thing to have duplicate tests but don't want to piss off our CI provider with unnecessary processing.
There was a problem hiding this comment.
Note you probably still need to call the train function, just not assert the result.
| @@ -0,0 +1,250 @@ | |||
| # try: # python 3 | |||
| # from apprentice_learner.models import Agent | |||
| # except ImportError: # python 2 | |||
There was a problem hiding this comment.
No need for this as we don't support python 2 actively.
| from apprentice.agents.ModularAgent import ModularAgent | ||
| from apprentice.agents.WhereWhenHowNoFoa import WhereWhenHowNoFoa | ||
| from apprentice.working_memory.representation import Sai | ||
| import dill |
There was a problem hiding this comment.
This is a style note but external imports should come before internal ones.
Removed redundant tests and tests that were only evaluating behavior of external libraries not related to our code. Fixed up the fo_planner test so that is equivalent to the numbert one, with the caveat that it does not verify structural equivalence because of lambdas. Removed dead code and cleaned up code style to bring it closer to PEP 8 spec.
eharpste
left a comment
eharpste
left a comment
There was a problem hiding this comment.
I've gone through the new files and added changes that mostly clean up the code and better address some of our testing needs.
2 participants
No description provided.