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

Fix the dtype of geometry data #612

Merged
merged 1 commit into from
Jun 8, 2023
Merged

Fix the dtype of geometry data #612

merged 1 commit into from
Jun 8, 2023

Conversation

lfarv
Copy link
Contributor

@lfarv lfarv commented Jun 3, 2023

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

@lfarv lfarv added Python For python AT code bug fix labels Jun 7, 2023
@lfarv lfarv mentioned this pull request Jun 7, 2023
@swhite2401
Copy link
Contributor

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

@simoneliuzzo
Copy link
Contributor

simoneliuzzo commented Jun 8, 2023

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
Copy link
Contributor Author

lfarv commented Jun 8, 2023

@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
Copy link
Contributor

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
Copy link
Contributor

Dear @lfarv,

I did the test and it works the same.

So ok for me.

@lfarv
Copy link
Contributor Author

lfarv commented Jun 8, 2023

@simoneliuzzo:

I did the test and it works the same.

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

@lfarv lfarv merged commit 42589ba into master Jun 8, 2023
31 checks passed
@lfarv lfarv deleted the bugfix_get_geometry branch June 8, 2023 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix Python For python AT code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants