glasgowcompbio / vimms

A programmable and modular LC/MS simulator in Python

MIT License

19 stars 6 forks source link

Persistent state in controllers... #180

Closed sdrogers closed 4 years ago

sdrogers commented 4 years ago

Based on previous discussion w.r.t how we preserve some kind of state from one injection to the next, wondering if we should develop the following...

A prioritisation object. A DDA controller, inside process_scan passes the scan it's processing to this object and it returns the list of tasks. This moves functionality out of the controller (which becomes more of a shell to interact with the environment) and creates a dedicated object that decides what the next action (or set of actions) ought to be.

Why? Well, our controllers are designed to only run once. But we want to preserve some state from one injection to the next. The same prioritisation object could be passed around to multiple, remembering what happened in previous injections, doing alignment etc, and keeping track of exclusion.

sdrogers commented 4 years ago

See examples (test_controllers_shell.py) in branch iss_180

joewandy commented 4 years ago

The codes look sensible, we could go with it.

Some comments below:

In reinforcement learning literatures, something that looks at states and decide what's the next action to take is usually called an 'agent'. We could use that terminology too rather than 'oracle', just to be consistent with what other people use?
In next_tasks we pass in the controller object. I'm guessing it's only because we need access to get_ms1_scan_params and get_ms2_scan_params inside the controller? Don't need to do this now, but I think it would be good to re-open this issue. and move those two methods out elsewhere.
I can guess how it works but it seems that test_controllers_shell.py is missing?

vinnydavies commented 4 years ago

Happy to go with this. It seems somewhat related to #15 / that issue could be addressed at the same time

joewandy commented 4 years ago

15 is for existing codes, right? There will be some code duplications (and also in functionalities) as we keep both the codes for the old-style controllers and the new oracle-/agent-based ones.

I guess eventually we hope to move away from all the old controllers to using the new style.

sdrogers commented 4 years ago

thanks both -- re the controller -- initially yes, that's why. But actually, maybe it's useful because the agent / oracle can keep track of what they asked each controller to do, which is helpful, even if the scan parameters are accessible elsewhere....

sdrogers commented 4 years ago

not so sure we need to remove the scan params from the controller object though -- it having scan params is not a bad thing. Perhaps just shouldn't be the only place?

Made some small changes in the iss_180b branch:

get_ms1_scan_params and get_ms2_scan_params stay as they are in the base controller, but inside they call these methods which are now moved to Common.py (previously they were in the environment).
Renamed 'oracle' to 'agent'.

joewandy commented 4 years ago

Created the folowing pull request https://github.com/sdrogers/vimms/pull/183 for all the above.

sdrogers commented 4 years ago

Shall we close now? Or keep this open for discussion?

joewandy commented 4 years ago

Closed