glasgowcompbio / vimms

A programmable and modular LC/MS simulator in Python
MIT License
19 stars 6 forks source link

The first scan should be made by the controller #52

Closed sdrogers closed 4 years ago

sdrogers commented 4 years ago

The first scan should be made by the controller and not by the MS.

At the moment, it comes from the MS and therefore can have completely different parameters to subsequent scans. This is a mess.

sdrogers commented 4 years ago

when fixed, please update this chunk (around line 150) in test_controllers.py:

        for scan_level, scans in controller.scans.items():
            for s in scans[1:]:
                assert min(s.mzs) >= min_mz
                assert max(s.mzs) <= max_mz

Once this issue is resolved, we can loop over all scans, not skip the first one.

sdrogers commented 4 years ago

Would like to see some movement on this if possible -- it's going to become a pain. Presumably somewhere in the python there is a line that creates the scan parameters for this scan. Could we fix by the controllers having a method get_initial_scan that is called instead of generating these default parameters? If the controller returns None then we'd get the behaviour we have now?

joewandy commented 4 years ago

Going to look at this now

sdrogers commented 4 years ago

It's a bit of a tangled one, so please discuss possible solutions here first so we have high chance of avoiding problems!

joewandy commented 4 years ago

Here's the plan:

  1. In the base Controller, we add a method get_initial_scan that returns the initial scan parameters. This defaults to the MS1 scan that we have now (with default params), although subclasses can override this to return whatever they want.

  2. In the simulated mass spec, we get rid of the line below that generates the initial scan (self.environment.get_default_scan_params()), and instead uses the initial scan params returned by the controller.

    def step(self, initial_scan=False):
        """
        Performs one step of a mass spectrometry process
        :return:
        """
    
        # get scan param from the processing queue and do one scan
        if initial_scan:
            params = self.environment.get_default_scan_params()
            is_method_scan = False
        else:
            params, is_method_scan = self._get_params()
        scan = self._get_scan(self.time, params)

    Also it's better if the sending of the initial scan is done upon receiving the acquisition open event, rather than using this initial_scan flag in the step method. This is already the case for the IAPI mass spec, so we just need to tidy it up on the simulated mass spec too.

  3. Finally in the IAPI mass spec, we should also use the initial scan params from the controller, rather than making a default one:

        # send the initial custom scan to start the custom scan generation process
        params = self.get_default_scan_params()
        self.mass_spec.send_params(params)
sdrogers commented 4 years ago

sounds good. Presumably the get_initial_scan in the base controller can use the params object passed into the constructor of the base controller? This holds all (I think) necessary scan parameters, and will revert to default if the user doesn't specify an AdvancedParams object?

joewandy commented 4 years ago

good idea, will do just that.

sdrogers commented 4 years ago

that'll mean no other controllers need to be changed, right?

sdrogers commented 4 years ago

but the first scan will match whatever the user is specifying

sdrogers commented 4 years ago

in the fixed sequence controller, we could override it to be the first scan in the sequence list?