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

Broadcasting in Ska.quatutil.yagzag2radec #13

Closed
javierggt opened this issue Sep 3, 2020 · 5 comments · Fixed by sot/chandra_aca#104
Closed

Broadcasting in Ska.quatutil.yagzag2radec #13

javierggt opened this issue Sep 3, 2020 · 5 comments · Fixed by sot/chandra_aca#104

Comments

@javierggt
Copy link
Contributor

This issue follows issue sot/chandra_aca/issues/102 and it is better described there.

@taldcroft
Copy link
Member

I have a fix for this which I believe works. Part of the problem is that we (I?) have chosen the wrong convention for where to place the extra axes in places (e.g. eci2radec). These extra axes were put at the end of the shape, which can make life easier if there is just a single extra axis, but for multiple extra axes I think that breaks down. For Quaternion @javierggt got it right.

def yagzag2radec(yag, zag, q):
    """
    Given ACA Y-ang, Z-ang and pointing quaternion determine RA, Dec. The
    input ``yag`` and ``zag`` values can be 1-d arrays in which case the output
    ``ra`` and ``dec`` will be corresponding arrays of the same length.

    :param yag: ACA Y angle (degrees)
    :param zag: ACA Z angle (degrees)
    :param q: Quaternion
    :rtype: list ra, dec (degrees)
    """
    try:
        one = np.ones(len(yag))
    except TypeError:
        one = 1.0
    d_aca = np.array([one, tan(radians(yag)), tan(radians(zag))])
    d_aca *= 1.0 / np.sum(d_aca**2)
    d_aca_t = np.moveaxis(d_aca, 0, -1)
    eci = np.einsum('...jk,...k->...j', q.transform, d_aca_t)
    eci_t = np.moveaxis(eci, 0, -1)
    return eci2radec(eci_t)

@taldcroft
Copy link
Member

One idea that just occurred to me is to basically stop updating Ska.quatutil and copy the functionality we need into Quaternion or chandra_aca.transform, and then make the N-d support work consistently. We could perhaps deprecate Ska.quatutil, but that is not necessarily needed.

@javierggt
Copy link
Contributor Author

I second this last proposal. I would split it: radec2yagzag and yagzag2radec into chandra_aca.transform, the rest into Quaternion.

@jeanconn
Copy link
Contributor

I may not understand scope, but I was thinking we might want to do something in Ska.quatutil to have those methods just not support a Quaternion with length > 1 and raise an error. That would at least be consistent. If we are moving methods around, we may still want something like that and a deprecation warning.

@javierggt
Copy link
Contributor Author

well, I think there is not much difference whether we make it work with the vectorized quaternion before or after deprecating and moving.

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 a pull request may close this issue.

3 participants