Skip to content

Conversation

@esc24
Copy link

@esc24 esc24 commented Feb 10, 2015

No description provided.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a tiny point, but I think this would all look simpler with some renames, so that these 2 lines would read (something like):

sqmag_10 = u_10_t**2 + v_10_t**2
sqmag_01 = u_01_t**2 + v_01_t**2

@pp-mo
Copy link
Owner

pp-mo commented Feb 10, 2015

The logic of this looks solid to me, but I don't really know how to justify the valid "magnitude accuracy limit"
( The code is currently using a tolerance of 1e-3 on the square magnitudes, so that's about 0.05% or 500ppm on the magnitude itself. Maybe this should be made clearer in the code... )

I guess the accuracy masking also itself needs a new test 😣

@pp-mo
Copy link
Owner

pp-mo commented Feb 10, 2015

This is not really in scope, but I just realised that the code is committing a serious inefficiency with repetition over slices...

Within the core code of 'transform_vectors', we derive 4 value arrays which define a '2x2 matrix' at each location, and apply these as the basic transform
(cf : https://github.com/esc24/iris/blob/f9359f3593bdaac1c6753ceca65e40efadc43ff9/lib/iris/analysis/cartography.py#L853

 # Transform vectors into the target system.
dx2_x1, dy2_x1, dx2_y1, dy2_y1 = _inter_crs_differentials(
src_crs, x, y, tgt_crs)
u2 = dx2_x1 * u1 + dx2_y1 * v1
v2 = dy2_x1 * u1 + dy2_y1 * v1

But these last two lines are the only use of the U and V values.

So the '_inter_cs_differentials' results (dx2_x1, dy2_x1, dx2_y1, dy2_y1) only depend on X and Y.
Which is equally true of the distance scaling factors "ds_dx1, ds_dy1, ds_dx2, ds_dy2".

So all of those arrays, which are actually the costly part of the calculation, are independent of Us and Vs, will be the same for each slice and so could in fact be computed just once, instead of once per slice.

@pp-mo
Copy link
Owner

pp-mo commented Feb 10, 2015

Thanks @esc24 .
Please let me know what you think of the comments.

Unfortunately, I'm still not sure we are done when we've conquered those points. I'd like to investigate the magnitude of the errors in some practical cases first, but perhaps you already have some examples, so we should confer on it..

@esc24
Copy link
Author

esc24 commented Feb 10, 2015

Happy to confer. I have a few ipython notebooks with data I can show you.

@esc24
Copy link
Author

esc24 commented Feb 11, 2015

I've left the optimisation out. It can be done at a future date if it proves to be an issue. After looking at a few different CRSs @pp-mo and I agreed to up the tolerance to 0.1%, and at the same time up the note in the docstring to a warning.

pp-mo added a commit that referenced this pull request Feb 12, 2015
added masking of erroneous values
@pp-mo pp-mo merged commit 074b1a1 into pp-mo:pp_vec_basis_change3 Feb 12, 2015
@pp-mo
Copy link
Owner

pp-mo commented Feb 12, 2015

Thanks @esc24
I'm quite happy with that.
I also checked out the other test code, as it basically all came from you originally and I hadn't fully checked it all before. Again, all good and all angles covered as far as I can see.
So, if you're happy, please merge the original.

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.

2 participants