labscript-suite-temp-2 / runmanager

runmanager is a graphical user interface (GUI) used to aid the compilation of labscript experiment scripts into hardware instructions to be executed on the hardware. Experiment parameters can be adjusted in the GUI, and lists of parameters can be used to create sequences of experiments, and scan over complex parameter spaces.
BSD 2-Clause "Simplified" License
0 stars 0 forks source link

Bugs with restoring expansion type when runmanager is reopened #50

Open philipstarkey opened 7 years ago

philipstarkey commented 7 years ago

Original report (archived issue) by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).


Following on from issue #49, there are two outstanding bugs I have found that I could not fix. These are:

I think that to fix these we will need to save some data as to whether the expansion type was set manually by the user or automatically by runmanager (and that this needs to be stored in the HDF5 file along side the expansion type). My reasoning for this is that we currently guess the expansion type which is either none or expansion, and then decide if it is a zip group. Because we transition from outer -> zip, we thus can't distinguish zip -> outer as it will appear like it's at the start of the transition from outer -> zip. Similarly, arrays that have had their expansion type removed, will have the expansion type guessed as "outer", since the expansion is determined from the type and so you can't tell the difference.

There may be a way to solve this (without storing extra data in the HDF5 file) by rebuilding the metadata used in the calculation of the expansion type, when runmanager restores open groups. However, I'm not 100% certain this is possible...

philipstarkey commented 6 years ago

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).


Ran into this bug today. I created 8 globals each storing a list of 20 values and set them from outer to nothing. Everything worked fine until I closed runmanager and reopened it to find my RAM going through the roof. This then resulted in a freeze of our lab machine which could only be resolved by the hard reset button.

This should really be fixed.

philipstarkey commented 6 years ago

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).


I'm a bit surprised by the PC crash. Do you have an example global file I can reproduce with?

As a possible workaround: I'm assuming you removed "outer" because you wanted a list of values in your labscript file? If so, wrapping the list in tuple(...) in runmanager may do the trick (or something similar that casts it to a tuple). runmanager only treats list and numpy.array as iterables.

philipstarkey commented 6 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).


Runmanager currently computes the number of shots by expanding the globals, so it would have been attempting to create a list of 20^8 = 25 billion elements, so I can see how that would fill system memory. Python should give a MemoryError, but perhaps the computer has swap space that it would have thrashed before then which could have made the computer unusable.

The expansion of the shots in preparse_globals() is solely to compute the number, which could of course be computed by multiplying together the axes lengths rather than doing the outer product and taking the length of the list. We should fix this too per performance reasons, even though that doesn't solve this bug.

philipstarkey commented 6 years ago

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).


Oh, that explains it!

We could fix the first half of this by enforcing that lists/numpy arrays are always treated as either outer or zip (and so expansions can't be empty) and that otherwise you must specify as a tuple or other list like data type. Are there any down sides to that really given we can use tuples for things that shouldn't be expanded?

The second half of the issue could be swept under the carpet by asking people to not change zip to outer but to a independently named zip group if they want it to persist across restarts, so that the name is not reverted (aka, leaving it as-is).

These changes would save us from having to store additional information in the HDF5 file.

Actually: might we be able to revisit the concept of "outer" entirely and replace it with a zip group containing a single global (at least as far as the backend is concerned)? Not sure exactly...but I think we get a few possible options if we enforce that list and numpy arrays must be expanded always.

philipstarkey commented 6 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).


I suspect we can just make runmanager not change the expansion types when it is starting up, can't we?

It doesn't change them every other time you change a global, it's just overzealous the first time.

guess_expansion_modes is trying to do this correctly:

#!python

    for group_name in evaled_globals:
            for global_name in evaled_globals[group_name]:
                new_value = evaled_globals[group_name][global_name]
                try:
                    previous_value = self.previous_evaled_globals[group_name][global_name]
                except KeyError:
                    # This variable is used to guess the expansion type
                    # 
                    # If we already have an expansion specified for this, but
                    # don't have a previous value, then we should use the 
                    # new_value for the guess as we are likely loading from HDF5
                    # file for the first time (and either way, don't want to 
                    # overwrite what the user has put in the expansion type)
                    #
                    # If we don't have an expansion...
                    # then we set it to '0' which will result in an
                    # expansion type guess of '' (emptys string) This will
                    # either result in nothing being done to the expansion
                    # type or the expansion type being found to be 'outer',
                    # which will then make it go through the machinery below
                    if global_name in expansions and expansions[global_name]:
                        previous_value = new_value
                    else:
                        previous_value = 0

But there must be a bug in here.

philipstarkey commented 6 years ago

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).


Using tuples solved the problem for me. I don't know that they don't expand.

I think it might be a good idea to get rid of non expanding lists and arrays. But we should document somewhere, that tuples will not expand and should be used in places where expansion is 'unwanted'.