-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
StationConfigurator #68
StationConfigurator #68
Conversation
@@ -155,14 +147,12 @@ def setup_parameter_from_dict(p, options): | |||
|
|||
# add the instrument to the station | |||
self.station.add_component(instr) | |||
|
|||
# restart the monitor to apply the changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this restart really needed anymore? Creating a new monitor should do the job?
Need microsoft/Qcodes#994 and microsoft/Qcodes#986 There is a experiments.db file committed that should probably be deleted |
|
||
def __init__(self, filename: str, station: Optional[Station] = None) -> None: | ||
self.monitor_parameters = {} | ||
# self.station = station if is not None else Station.default if is not None else Station() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the line above
def snapshot(self, update=True): | ||
return self.data | ||
|
||
# self.station.add_component(ConfigComponent(self.config), 'Config') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove?
|
||
# local function to refactor common code from defining new parameter | ||
# and setting existing one | ||
def setup_parameter_from_dict(p, options): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could use a better variable name that p. Typing would be nice too
d95450d
to
9cc9f75
Compare
# config from the qcodesrc.json file (working directory, home or qcodes dir) | ||
auto_reconnect_instrument = qcodes.config["user"]["auto_reconnect_instrument"] | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the atexit register logic be added to the station Configurator? Since it creates the instruments I think so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather prefer not to. The way it is now the StationConfigurator is an optional component, that you can use if you like it. The atexit should not be optional. But I have to say that I don't have a better idea where to put it.
What about the init of qdev wrappers. It might get added multiple times then but this should not break anything. Now that we are excluding packages from the UMR this should work, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes running it multiple times should be fine. We still need to merge microsoft/Qcodes#1009 to fix the UMR issue
…etup Feat/station configurator add (partial) YML file for test setup
…dev-wrappers into feat/StationConfigurator
@Dominik-Vogel Could you update the example yml for the test setup to the new style too |
@Dominik-Vogel Somehow a non standard char ended up on the last line of the testsetup config. Confusing git and pyyaml. I have pushed a fix to remove that |
@Dominik-Vogel can we call force_close_existing_instrument for enable_forced_reconnect? Having 'enable' in the name emphasizes that the setting allows you to reconnect instruments not that the code does anything on its own. |
@jenshnielsen Had I missed anything in the example yaml for the test setup? What do you want me to update? |
self.monitor_parameters = {k:v for k,v in self.monitor_parameters.items() if v.root_instrument is not instr} | ||
instr.close() | ||
# remove instrument from station snapshot | ||
self.station.components.pop(instr.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If understand this correctly then if enable_forced_reconnect
is false then the configure instead creates a new instrument in the station.
Is this the behavior we want? I initially assumed the configure would throw an error message and do nothing if enable_forced_reconnect
is false. Maybe we can add a warning message printed to the console to notify the user that a new instrument instance is created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's going to fail earlier because it's trying to create a new instrument with the same name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good to see that you already got so much used to the StationConfigurator, that you forgot how it was like before ;-). When you try to create the instrument again, you will get an error from qcodes saying that you can't create an instrument with the same name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comments crossed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I see. Thanks.
setup.py
Outdated
@@ -20,7 +20,8 @@ | |||
install_requires=[ | |||
'matplotlib>=2.0.2', | |||
'pyqtgraph>=0.10.0', | |||
'qcodes>=0.1.3' | |||
'qcodes>=0.1.3', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be bumped to the latest release?
One more request: Would it be possible to add Offset to the list of PARAMETER_ATTRIBUTES. I have a specific instrument that really could benefit from this. I realise that this is probably more a question for parameter class in core qcodes but if it is not too hard to add I think it would be a nice feature. This would allow the user to easily adjust for calibration errors on instruments (like a constant voltage offset that is often seen on decadacs), |
You are totally right, and this is already on my list of things to implement. |
@Dominik-Vogel can you add a function for updating the monitor? That is running .get() on all parameters in the monitor list. |
sure :-) |
@Dominik-Vogel maybe we can merge this one and then add extra functionality in future PR's? |
In this PR a
StationConfigurator
object is introduced, which enables loading instrument through a yaml file. An example code could look like this:With a yaml file containing:
For the complete example and an extensive commented yaml file take a look at the example in the example folder. For a full list of features refer to the yaml file.
This PR depends on microsoft/Qcodes#986
Current issues: