slgentil / croco

Code CROCO de base + CONFIGS qui contient les différentes configurations
0 stars 1 forks source link

interp2z consolidation and improvement #18

Closed apatlpo closed 4 years ago

apatlpo commented 4 years ago

The goal of this PR is to interact around a consolidation and improvements of the vertical interpolation routine interp2z.

apatlpo commented 4 years ago

List of cases we may encounter when interpolating data vertically:

We probably need also to add a wrapper around interp2z ("à la" z2zmoy).

apatlpo commented 4 years ago

Just did a good first pass through C/numpy code. I actually wondered how it was working at all previously with target vertical grids that were not 1D. Next step is to work on xarray wrapper which will require some work on alignment.

apatlpo commented 4 years ago

pinging @NoeLahaye in case you have not seen this PR yet.

apatlpo commented 4 years ago

this is problably close to ready for you to look at @NoeLahaye

I am not entirely clear about core dimension treatment within interp2z. If this clear to you, please let me know.

NoeLahaye commented 4 years ago

When using chunks, I had an error of the form: ValueError: Chunks and shape must be of the same length/dimension. Got chunks=(), shape=(2,). I don't have it anymore if I pass (z_pos, z_dim)as kwargs to interp2z_np, and the routine executes without failing.

I think there are still some issues with broadcasting / aligning. I have been having errors in some configurations by interp2z_np_3d because of mismatching shapes between v and z_t. The behaviour we want is: inner alignment along every dimensions (present in all data arrays) except the vertical one for the target z_t, broadcasting for any other dimension. I'm not sure any longer how this is being dealt with by apply_ufunc. I need some more work on this.

apatlpo commented 4 years ago

good job @NoeLahaye you caught a good number of the bugs.

(z_pos, z_dim) has to be passed as a kwarg indeed, apply_ufunc args can only be array like objects.

If you have a specific example of an error with interp2z_np_3d could you please push a snippet reproducing the error so that I can look at it? Have you actually recompiled the C code which was updated?

Also, don't feel you have to test or investigate anything yourself. Just look at the code and point toward suspicious points or suggests tests and I'll execute.

apatlpo commented 4 years ago

I believe I have made good progress, there was still much to be done.

The test notebook need to be a bit improved and address the case of same vertical dimension interpolation.

apatlpo commented 4 years ago

I believe this is ready for a final review @NoeLahaye The notebook you need to look at is interp2z.ipynb

Unless you see any obvious bugs and improvements, I'd be up to then merge and adjust later the code with our real case scenarios on branch master. It's not good to have PR that are too massive and this one starts being significant.

NoeLahaye commented 4 years ago

It looks like we cannot "expand" an interpolated array on additional dimension: it is just the final transposition that fails.

p = get_dimension_permutations(1)[0]
pt = get_ztarget_dimension_permutations(3)[0]
test_interp2z(*get_synthetic_data(p, zt_p=pt))

tested in interp2z.ipynb gives the following error (just copying the last few lines):

ValueError: arguments to transpose (('z_target1d',)) must be permuted array dimensions (('y', 'x', 'z_target1d'))

raising at line 449:

vout = vout.transpose(*[_zt_dim if d==z_dim else d for d in list(v.dims)])

NoeLahaye commented 4 years ago

In interp2z.ipynb, every cases have 1 dimensional z. The case where z has the same dimensions as v should be tested (although I think it will work just as fine)

apatlpo commented 4 years ago

I had not been thinking at all about cases when zt may have more dimensions than z and v, thanks for pointing that out. The final transposition has been adjusted to account for this case. Note that I added an extra keyword argument to control the dimension order of the final DataArray.

The notebook has been updated to test for these last cases.

Note also, that I had to update all my librairies to get several tests to go through (suspect issue with tornado which takes care of all communications).

Ok to merge?

NoeLahaye commented 4 years ago

OK to merge !

apatlpo commented 4 years ago

let's go then, thanks @NoeLahaye you did the job !