Fix station snapshot of parameters with snapshot_exclude

[mgunyho]

Fixes #2671

However, there's still one issue: the line

station.add_component(excluded_parameter)

in the test still issues a warning, because add_component takes a snapshot, because of update_snapshot=True. How should this be handled?

I first thought that maybe it's the caller's responsibility to pass update_snapshot=False when adding an excluded parameter like this, but this won't work then when initializing the Station as Station(param1, param2, excluded_param, param4...).

So maybe there could be a similar check in add_component for isinstance(component, Parameter) and not component.snapshot_exclude, but that also feels a bit clumsy.

@astafan8

[codecov]

Codecov Report

Merging #2692 (783115d) into master (01506d0) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2692      +/-   ##
==========================================
- Coverage   63.86%   63.84%   -0.02%     
==========================================
  Files         198      198              
  Lines       26487    26489       +2     
==========================================
- Hits        16916    16913       -3     
- Misses       9571     9576       +5     

[jenshnielsen]

@mgunyho thanks for the pr.

in the test still issues a warning, because add_component takes a snapshot, because of update_snapshot=True. How should this be handled?

I first thought that maybe it's the caller's responsibility to pass update_snapshot=False when adding an excluded parameter like this, but this won't work then when initializing the Station as Station(param1, param2, excluded_param, param4...).

So maybe there could be a similar check in add_component for isinstance(component, Parameter) and not component.snapshot_exclude, but that also feels a bit clumsy.

I don't see a better solution than checking this in add_component too. You are probably seeing these issues since the usecase of directly adding parameters to the station is not particularly commonly used. In most cases users will add an instrument to the station and the snapshot of the instrument will take care of the filtering.

[mgunyho]

Alright, I added it now. It's true that this is a bit of an edge case. We are using some custom Station subclasses where this came up.

[astafan8]

@mgunyho custom station classes? :) that's interesting! could you share what these classes do and what is their goal? (we could also do it on slack if you prefer)

[mgunyho]

@astafan8 We use station subclasses for a couple of purposes.

• The subclass is specific to an experiment, and it has a load_instruments method that loads a specific set of instruments from the config. It also initializes some metainstruments (that require loading physical instruments before they can be instantiated), and also some experiment-wide "global" parameters that are not bound to any instrument but only to the station (that's where this issue came up).
• We have a complicated custom version of the doNd sweep function as a method of the subclass. The subclass has an experiment attribute so we don't always have to do station.do_sweep(..., exp=exp).

I'm not sure if we are doing things in the most optimal way, but it seems to work reasonably well for now.

[astafan8]

@mgunyho thank you for sharign! the first point makes sense and is an itneresting case; the folks i'm working with usually use separate yml files and anyway load all instruments from them. For the second point, also fair, however, this will be better solved (and we have it on our radar, just haven't got to doing it) by introducing a concept/variable of active experiment that you set once, and doNds and what not can pick it up (at the moment, donds and also Measuremnt class pick the "last" experiment which may well be the "active" one but also may not).