Skip to content
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

Driver/Mercury iPS with VISA #897

Merged

Conversation

WilliamHPNielsen
Copy link
Contributor

@WilliamHPNielsen WilliamHPNielsen commented Dec 4, 2017

Add a VISA driver for the MercuryiPS using the FieldVector setpoint technology already in use in our AMI430 driver.

Fixes #543

Changes proposed in this pull request:

  • Rewrite the MercuryiPS driver to use VISA
  • Make the driver use the FieldVector to deal with singularities

Work items:

@jenshnielsen

Add a working VISA driver with a single non-trivial parameter
@codecov
Copy link

codecov bot commented Dec 19, 2017

Codecov Report

Merging #897 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #897      +/-   ##
==========================================
+ Coverage   80.38%   80.48%   +0.09%     
==========================================
  Files          49       49              
  Lines        6801     6804       +3     
==========================================
+ Hits         5467     5476       +9     
+ Misses       1334     1328       -6

@WilliamHPNielsen
Copy link
Contributor Author

Note: development of this driver is paused until an agreement on the desired magnet ramping behaviour has been reached.

@WilliamHPNielsen
Copy link
Contributor Author

WilliamHPNielsen commented May 9, 2018

From talking to scientists, it seems that

  • Using the "first-down-then-up" approach of the AMI430 driver
  • Adding an optional ramp_resolution parameter for staircasing non-cartesian ramps

will suit the needs of everyone.

Development resumed!

@WilliamHPNielsen WilliamHPNielsen changed the title Driver/Mercury iPS with VISA [WIP] Driver/Mercury iPS with VISA Jun 11, 2018
lh.setFormatter(formatter)


def get_messages(iostream: io.StringIO) -> List[str]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use pytests capture log for this i.e. https://docs.pytest.org/en/latest/logging.html#caplog-fixture ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL! ^^

@WilliamHPNielsen WilliamHPNielsen changed the title [WIP] Driver/Mercury iPS with VISA Driver/Mercury iPS with VISA Jul 20, 2018
@WilliamHPNielsen
Copy link
Contributor Author

There are many extra features we can think of adding (one example: rotated coordinate systems to account for sample misalignment), but I think the driver is now in a functional state.

@QCoDeS/core

default: 0
getter:
q: "READ:DEV:GRPX:PSU:SIG:FSET"
r: "{}" # should there be a unit appended? How does the instrument do it?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the answer to this question known now?

Copy link
Contributor Author

@WilliamHPNielsen WilliamHPNielsen Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. It is known.

@@ -190,14 +243,73 @@ def __init__(self, name: str, address: str, visalib=None,
# to ensure a correct snapshot, we must wrap the get function
self.IDN.get = self.IDN._wrap_get(self._idn_getter)

# TODO: Query instrument to ensure which PSUs we have
# TODO: Query instrument to ensure which PSUs are actually present
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this an "issue" or "nice-to-have"? in other words, will it be done in this PR? If not, perhaps, make an issue out of it?

y=self.GRPY.field(),
z=self.GRPZ.field())

for coord in ['x', 'y', 'z', 'r', 'theta', 'phi', 'rho']:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this list be referred to from FieldVector class or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not so easily. Also, I do not suspect that list to change very much in the near future, so it's prolly not worth the effort.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@WilliamHPNielsen
Copy link
Contributor Author

Status: I am currently waiting for feedback from a scientist who kindly volunteered to try out this new driver.

@WilliamHPNielsen
Copy link
Contributor Author

@QCoDeS/core, this driver version has now been tested at a real lab station and allegedly worked fine and solved two problems with the older driver (the problem described in #543 and a problem with wrong metadata, both basically solved by using FieldVector).

So I'd say we merge and actually also deprecate the older driver.

@WilliamHPNielsen
Copy link
Contributor Author

WilliamHPNielsen commented Aug 3, 2018

I'll merge this now, but not deprecate the old driver just yet. The reason is that someone is using the old driver with two axes being MercuryPS Slaves and the third one being a less noisy power supply. I will add that functionality to the MercuryiPS VISA driver in a separate PR and then we can finally get rid of the old, untrusted IPInstrument driver (and maybe IPInstrument altogether?).

@WilliamHPNielsen WilliamHPNielsen merged commit 36a75ea into microsoft:master Aug 3, 2018
giulioungaretti pushed a commit that referenced this pull request Aug 3, 2018
Merge: b2344b7 658e591
Author: William H.P. Nielsen <[email protected]>

    Merge pull request #897 from WilliamHPNielsen/driver/MercuryiPS_VISA
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants