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

_parse_array may be too lenient, allowing users to input arrays of the wrong shape #3066

Closed
OceanNuclear opened this issue Mar 1, 2024 · 1 comment · Fixed by #3071
Closed
Labels
discussion issue to be converted to a discussion geometry Tasks relating to the geometry module

Comments

@OceanNuclear
Copy link
Contributor

Expected behaviour

Consider a bunch of xyz coordinates p1, p2, p3, ... etc.

Coordinates(np.array([p1,p2,p3,p4]).T) should return 4 points, while

Coordinates(np.array([p1,p2,p3,p4])) should give an error/warning.

OR vice versa. I don't mind. But I think one of these two methods should fail.

Actual behaviour

Coordinates(np.array([p1,p2,p3,p4]).T) returns 4 points, while

Coordinates(np.array([p1,p2,p3,p4])) returns the SAME 4 points.

BUT

Coordinates(np.array([p1,p2,p3]).T) returns 3 points (along with a bluemira warning) while

Coordinates(np.array([p1,p2,p3])) returns the wrong (or right, depending on how you want to define it) 3 points (along with the same bluemira warning).

Explanation

When a new instance is created, Coordinates uses _parse_array to check that the shape of the array is correct.

It forces the array into the correct shape if it isn't:

n, m = xyz_array.shape
if m == DIM:
    xyz_array = xyz_array.T

Impact

This is fine when n!=m. However, when I'm making a script that uses a variable number of points, then when the number of points = 3, the resulting script will behave very differently/break, creating a difficult-to-reproduce bug.
And I can conceive situations where the test fails to catch this, from my personal experience:

I was using make_polygon to make a BluemiraWire with a variable number of points. And I had been using it the wrong way (Coordinates(np.array([p1,p2,p3,p4,p5...])) with no errors or warnings from bluemira. But there are places in the script that I haven't tested yet that may use exactly 3 points. If I wrote one test for both branches of the script then I might miss the errors that arise in the 3-point case.

Suggested actions

  1. I don't know how much code hinges on bluemira.geometry.coordinates.Coordinates. Maybe the first step would be to identify how much this change would impact, if we decide to remove the xyz_array = xyz_array.T line.
  2. Maybe it's worth making a discussion encompassing both this issue and Minor docstring/behaviour mismatch in 2D and 3D coordinates/geometries. #3040 ?
  3. Or if we agree to make a change, keep this behaviour for at least 1 release, and raise a DeprecationWarning, then change it in the next release?
@OceanNuclear OceanNuclear added geometry Tasks relating to the geometry module discussion issue to be converted to a discussion labels Mar 1, 2024
@OceanNuclear OceanNuclear changed the title _parse_array may be too lenient, allowing users to input arrays of the wrong shape, and may lead to unfound errors _parse_array may be too lenient, allowing users to input arrays of the wrong shape Mar 1, 2024
@je-cook
Copy link
Contributor

je-cook commented Mar 4, 2024

We aim to be as unintrusive as possible therefore this is desired behaviour:

Actual behaviour

Coordinates(np.array([p1,p2,p3,p4]).T) returns 4 points, while

Coordinates(np.array([p1,p2,p3,p4])) returns the SAME 4 points.

We warn for 3x3 arrays because we cannot detect which dimension is for x,y,z and which is for a singe coordinate. In the 3 dimensional case you need to ensure that x, y, z = np.array([...]) ie np.array([]).shape[0] == axes. If this is not explicit enough for you it is possible to do Coordinates({'x': np.array(), 'y': np.array(), 'z': np.array}). While your point about the normal vector in #3040 (the input probably should be 2, N) is correct Coordinates has a get_normal_vector function .

We could clarify this in some docstring, although I thought it was there. We can talk about #3040 in that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion issue to be converted to a discussion geometry Tasks relating to the geometry module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants