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

Mercury fixes #573

Merged
merged 6 commits into from
Apr 18, 2017
Merged

Mercury fixes #573

merged 6 commits into from
Apr 18, 2017

Conversation

jenshnielsen
Copy link
Collaborator

Some small issues found while working with the code on T10

@MerlinSmiles @QCoDeS/core

The relevant arrays are one element arrays not 3 element arrays so we need to take the first element to get a scalar
@MerlinSmiles
Copy link
Contributor

@jenshnielsen are you sure that your changes of [self.axes.index(ax)] to [0] actually work?
the _ATOB makes setpoint and field a len(ax) array, no?

@MerlinSmiles
Copy link
Contributor

while youre at it, maybe we want to change this function:

    def _carttosphere(self, field):
        field = np.array(field)
        r = np.sqrt(np.sum(field**2))
        if r == 0:
            theta = 0
            phi = 0
        else:
            theta = np.arccos(field[2] / r)
            phi = np.arctan2(field[1],  field[0])
            if phi<0:
                phi = phi+np.pi*2
        return [r, theta, phi]

basically adding:

            if phi<0:
                phi = phi+np.pi*2

to make sure phi is always positive?

@MerlinSmiles
Copy link
Contributor

and maybe we want to define if we run the magnet in current or field mode, and then remove all the ...C guys

@jenshnielsen
Copy link
Collaborator Author

@MerlinSmiles you are probably right but unfortunately it doesn't take that code path for other parameters such as z field

@MerlinSmiles
Copy link
Contributor

now I see the issue, yes its only going there for the 'current' version of sending the field. Stupid mistake.

A note on the side, using the current parameters instead of tesla parameters gives you one decimal place higher in resolution on the magnet.

@jenshnielsen
Copy link
Collaborator Author

Agreed with @MerlinSmiles in Slack

@jenshnielsen jenshnielsen merged commit 61babe0 into microsoft:master Apr 18, 2017
@jenshnielsen jenshnielsen deleted the mercury_fixes branch April 18, 2017 08:49
giulioungaretti pushed a commit that referenced this pull request Apr 18, 2017
Author: Jens Hedegaard Nielsen <jenshnielsen@gmail.com>

    Mercury fixes (#573)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants