atcollab / at

Accelerator Toolbox
Apache License 2.0
48 stars 31 forks source link

Fix the dtype of geometry data #612

Closed lfarv closed 1 year ago

lfarv commented 1 year ago

The data type of geometry data (output of Lattice.get_geometry) is incorrect:

geodata.x has shape (n, 1) instead of (n,) geodata[0].x is a (1,) array instead of a scalar

This PR fixes that, but has consequences on existing code making use of this data.

In most cases, the present output needs a squeeze(), which would still work though now useless. In other cases code will break

Keeping get_geometry unchanged is of course possible, at the cost of unnecessary complexity of user code…

Please advise !

Note for @simoneliuzzo : this appeared when trying to implement a GeometryObservable

swhite2401 commented 1 year ago

For me this is ok, but @simoneliuzzo may have a word on this

simoneliuzzo commented 1 year ago

Dear @lfarv and @swhite2401,

I am using get_geometry in these days. I do not find issues with the data type. It seems reasonably similar to the matlab one. I do not use it as @lfarv mentions.

For example I use it as:

    def get_fin_pos(t):
        geom, _ = at.get_geometry(t)
        return geom['x'][-1], geom['y'][-1], geom['angle'][-1]

and

geom_old, _, ax = old_sector.plot_geometry()
start_coordinates = copy.deepcopy(geom_old[0])
start_coordinates['x'] = 0.0

would this pull request change these commands?

In case it would be an API change, thus it would have to be scheduled and documented for the next MAJOR release.

lfarv commented 1 year ago

@simoneliuzzo:

What changes is the data type. I take your example (with my data…)

With the current release:

1st example:

>>> return return geom['x'][-1], geom['y'][-1], geom['angle'][-1]
>>> geom['x'][-1]
array([26.18099968])
>>> type(geom['x'][-1])
numpy.ndarray

You get an array with one component, instead of a floating point number ! Second example:

>>> geom_old, _, ax = old_sector.plot_geometry()
>>> start_coordinates = copy.deepcopy(geom_old[0])
>>> start_coordinates
([26.18099968], [-2.5786034], [-0.19634954])
>>> start_coordinates['x'] = 0.0
>>> start_coordinates
([0.], [-2.5786034], [-0.19634954])

Here you get a record (x, y, angle), but each item is itself an array with one component.

Now the same examples with the bug corrected:

>>> return return geom['x'][-1], geom['y'][-1], geom['angle'][-1]
>>> geom['x'][-1]
26.180999682232297
>>> type(geom['x'][-1])
numpy.float64

Now you get a floating point number, that's how it should be. Well, technically, it is in fact a so-called scalar array (an array with 0 dimensions!) , but it behaves exactly like a float.

Second example

>>> geom_old, _, ax = old_sector.plot_geometry()
>>> start_coordinates = copy.deepcopy(geom_old[0])
>>> start_coordinates
(26.18099968, -2.5786034, -0.19634954)
>>> start_coordinates['x'] = 0.0
>>> start_coordinates
(0., -2.5786034, -0.19634954)

The record now contains three floating point numbers.

Consequences

It depends what you are doing with these numbers. In python, an array with one component is totally different from a scalar number. That's why correcting the bug makes all the following processing much easier.

But if on your side you applied some tricks to extract the scalars from the array, it may change. The consequences depend upon what you do later. For instance, multiplication or addition of an array with a scalar still gives an array (as in Matlab), so maybe you did not even notice the bug and worked with these arrays without noticing. In which case the correction will be transparent. Or not…

simoneliuzzo commented 1 year ago

Dear @lfarv,

I use the code in a matching script, and it works. This is why I do not see the need to change it. If it works without any modifications on my side I will agree to merge. I need to pull this branch and create a new venv to test it. I will send you the output.

simoneliuzzo commented 1 year ago

Dear @lfarv,

I did the test and it works the same.

So ok for me.

lfarv commented 1 year ago

@simoneliuzzo:

I did the test and it works the same.

That was my hope, but it could not be guaranteed. Anyway, thanks for testing !