fix ra0/roll0 shape

[javierggt]

Should fix issue #21

• [x] cast input ints to floats
• [x] add tests for types

[taldcroft]

Yes to both. #20 showed that np.array(0) is not acceptable, and surely type(q.ra) is type(q.ra0) should always be true. And same for all the other similar equatorial attributes.

[jeanconn]

Regarding testing type, I suppose that should apply to every test that currently checks shape.

[taldcroft]

Np.float64 would also be acceptable, but the point is that the type should be consistent.

[javierggt]

I just made the fix for the return type. Do you also want to test for that? To me, whether the return type is float, np.float64 or dim-zero nparray with dtype np.float64 is a cosmetic issue (I fixed it anyway, so the question is if we have to test for it).

About replacing q = Quat(equatorial=equatorial_23[0, 0]) by q = Quat([10, 20, 30])...

In the tests, equatorial_23 is used about 20 times. transform_23 is uses about 15 times. q_32 is used about 30 times. Some of those are similar to this case. I think replacing all similar uses to the explicit numbers actually does not increase readability. I believe you can find similar lines, and in other PRs you could make similar comments about lines that happen to be next to the lines relevant for the PR.

[taldcroft]

To me, whether the return type is float, np.float64 or dim-zero nparray with dtype np.float64 is a cosmetic issue

zero-dim ndarray is a completely different beast from float and np.float64. The obvious example is the one we know:

In [1]: x = np.array(0.0)

In [2]: f'{x:.2f}'
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-f252eff5e93b> in <module>()
----> 1 f'{x:.2f}'

TypeError: unsupported format string passed to numpy.ndarray.__format__

In [3]: x = np.float64(0.0)

In [4]: f'{x:.2f}'
Out[4]: '0.00'

For a widely-used upstream package these things matter!

Another difference between types is JSON serialization:

In [7]: x = np.array(0.0)
In [8]: json.dumps(x)
---------------------------------------------------------------------------
...
~/miniconda3/envs/ska3/lib/python3.6/json/encoder.py in default(self, o)
    178         """
    179         raise TypeError("Object of type '%s' is not JSON serializable" %
--> 180                         o.__class__.__name__)
    181 
    182     def encode(self, o):

TypeError: Object of type 'ndarray' is not JSON serializable

In [10]: x = np.float64(0.0)
In [11]: json.dumps(x)
Out[11]: '0.0'

There are certainly many other places where the fact that np.float64(0.0) is a subclass of Python float is important and will break code if not honored, starting with methods of float that don't exist on ndarray.

Upshot is that a function that accepts an input that can be a scalar (float or subclass) or array and returns an output of the same dimensionality should (generally speaking) return a corresponding scalar (float or subclass) or array.

tl;dr - please test the types to ensure correct functionality now and ensure no future regressions.

[taldcroft]

About replacing q = Quat(equatorial=equatorial_23[0, 0]) by q = Quat([10, 20, 30])...

It depends on the context of the test. In this particular case the test is all about testing the shape of the output for a particular shape of input. The way the current test is written, I had to first find the definition of equatorial_23 by viewing the file on GitHub, then carefully count brackets to decide what is its shape. Maybe because you wrote this you knew instantly that equatorial_23 has shape (2,3), but in reviewing this test I did not and went through the process described. If instead it was written equatorial=[10, 20, 30] then I would known instantly instead of a 30 seconds later, which for me makes it easier to read.

I agree that most of the other tests are fine as written.

[taldcroft]

BTW, I discovered this trick of numpy in the detailed notes: "An empty (tuple) index is a full scalar index into a zero dimensional array. x[()] returns a scalar if x is zero dimensional and a view otherwise. On the other hand x[...] always returns a view.". Not sure but maybe it is helpful:

In [27]: type(np.array(0)[()])
Out[27]: numpy.int64

In [28]: type(np.array([0,1])[()])
Out[28]: numpy.ndarray

[jeanconn]

I saw that at https://github.com/sot/acdc/blob/5f57cef8eea9ae622e427aaa6dc9e377c0e111bc/acdc/check_cal.py#L87

[taldcroft]

I saw that at ...

Though the intent there was simply to force making a view of the FITS data array attribute, which would disappear otherwise. The behavior of automatically turning an ndarray with shape=() into a scalar like np.float64 was new to me.

[javierggt]

This solution would make types consistent between themselves and with the input type, and it would also remove some if statements. I will make the changes.

There is still the question of whether you want to enforce a type. Types do change after some operation. For example, q = Quat(equatorial=[10,10,10]) creates a quaternion with integer equatorial representation, but the other representations are still float.

An integer equatorial representation does not make much sense anyway.

[jeanconn]

So we've decided to cast to floats for the equatorial(10, 20, 30) case. And then needs tests.

[taldcroft]

I pushed a commit to coerce inputs to float64 and test for shapes and types. I'm sure the tests are partially redundant with existing tests, but nobody ever complained about too many tests. I think this should be good now?

[javierggt]

I'm ok with it.