Skip to content

Conversation

@corinnebosley
Copy link
Member

When two cubes have coordinates which do not match, an error should be raised if a user tries to perform arithmetic operations on them. These cases are caught by the add and subtract operators, but not the multiply and divide operators.

This PR contains tests for all the operators on a) a pair of cubes with totally non-matching coordinates and b) a pair of cubes with one coordinate array reversed.

It also extends the behaviour of add and subtract to apply to multiply and divide.

@corinnebosley
Copy link
Member Author

The user who reported this bug requested that iris catch the case of reversed coordinates and work out that the coordinates match up, but one array is reversed, and simply deals with this accordingly.

I believe that this is not what iris should be doing, so I have opted to catch the case and raise an error instead, letting the user adjust the cube themself.

@corinnebosley
Copy link
Member Author

@marqh Default test on last run timed out. Apart from that, everything seems to pass OK.

@marqh
Copy link
Member

marqh commented May 3, 2017

@corinnebosley this looks good, thank you

@marqh marqh merged commit e72d17f into SciTools:master May 3, 2017
@QuLogic QuLogic added this to the v2.0 milestone May 3, 2017
@QuLogic QuLogic modified the milestones: v1.13.0, v2.0 May 17, 2017
@djkirkham
Copy link
Contributor

It has been reported that this is causing issues with user code. Is it worth releasing 1.13.1 with this change taken out?

@corinnebosley @marqh @pelson

@rcomer
Copy link
Member

rcomer commented Jun 12, 2017

Hello,

it was me that reported issues as my code is breaking at 1.13. There is a relevant issue (#1887) and PR (#1911) with some discussion about what tests would be sensible. Unfortunately we never came to any firm conclusions!

Ruth

@corinnebosley
Copy link
Member Author

Hi @rcomer, thanks for your feedback, I have raised an issue (see this ticket) regarding the behaviour of math ops. I agree that the current behaviour is not quite right, but I also think that it should be consistent between add/subtract and multiply/divide, so we need to decide as a team what the behaviour should be. I have added this to the current project so that we discuss this issue within the dev team and make sure that it gets addressed very soon.

In the meantime, you can use module load scitools/default-previous to use Iris 1.12 instead of 1.13 to be compatible with your code.

What would be really useful is if you could put together a few small use cases for us (like the gravity one in this comment) that we can use to make a decision on behaviour and implementation.

I will make sure that you are notified about any changes in this area so that we can get your opinion and suggestions. Thanks!

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 this pull request may close these issues.

5 participants