MetOffice / opsinputs

JEDI library generating VarObs and Cx files
BSD 3-Clause "New" or "Revised" License
4 stars 1 forks source link

Add files for radar Doppler wind processing #212

Closed ctgh closed 4 months ago

ctgh commented 4 months ago

Add files for radar Doppler wind processing:

Fixes #211

ctgh commented 4 months ago

Thanks for both of your reviews! I will respond today.

ctgh commented 4 months ago

@ReubenHill I have implemented all of your suggestions in a662ba6.

ctgh commented 4 months ago

In 591e7f4 I updated deps/ops/stubs/OpsMod_Varobs/Ops_VarobPGEs.inc in order to write out the radial wind PGEs correctly. The main issue with the existing code is that the logical variable RadWind_SuperOb will never be true in opsinputs (whereas it is true in OPS). I decided to remove the test on that variable and ensure that the correct PGE is written out.

@mikecooke77 As mentioned in the comment above, I have modified one of the OPS stubs because it was leading to inconsistent behaviour relative to the actual OPS code. This change should not be ported into OPS itself. Do you foresee any issues with doing that?

mikecooke77 commented 4 months ago

In 591e7f4 I updated deps/ops/stubs/OpsMod_Varobs/Ops_VarobPGEs.inc in order to write out the radial wind PGEs correctly. The main issue with the existing code is that the logical variable RadWind_SuperOb will never be true in opsinputs (whereas it is true in OPS). I decided to remove the test on that variable and ensure that the correct PGE is written out.

@mikecooke77 As mentioned in the comment above, I have modified one of the OPS stubs because it was leading to inconsistent behaviour relative to the actual OPS code. This change should not be ported into OPS itself. Do you foresee any issues with doing that?

The issue comes when you try and update the code for changes to the OPS e.g. we have a new datatype. If you copy straight from OPS changes like this will disappear. At one point we were thinking of having some sort of "copy fixes back in" kind of functionality but I don't think this is ever used.

ctgh commented 4 months ago

In 591e7f4 I updated deps/ops/stubs/OpsMod_Varobs/Ops_VarobPGEs.inc in order to write out the radial wind PGEs correctly. The main issue with the existing code is that the logical variable RadWind_SuperOb will never be true in opsinputs (whereas it is true in OPS). I decided to remove the test on that variable and ensure that the correct PGE is written out. @mikecooke77 As mentioned in the comment above, I have modified one of the OPS stubs because it was leading to inconsistent behaviour relative to the actual OPS code. This change should not be ported into OPS itself. Do you foresee any issues with doing that?

The issue comes when you try and update the code for changes to the OPS e.g. we have a new datatype. If you copy straight from OPS changes like this will disappear. At one point we were thinking of having some sort of "copy fixes back in" kind of functionality but I don't think this is ever used.

I see. Presumably I could a test to cover this eventuality (i.e. check the output PGEs are as expected). If the file is ever overwritten the test will start to fail. Do you know if this version of OPS code has been updated?

Failing that, I might be able to hack things in the varobswriter. I'll take a look at that too.

ctgh commented 4 months ago

In fa19be4 I removed the modification to the OPS code. Instead both Ob % RadialVelocSO and Ob % RadialVelocity are filled. It's a bit hacky but causes the PGE to be filled correctly. The unit tests pass, but I am also checking the full end-to-end test. I will let you know how it goes...

ctgh commented 4 months ago

In fa19be4 I removed the modification to the OPS code. Instead both Ob % RadialVelocSO and Ob % RadialVelocity are filled. It's a bit hacky but causes the PGE to be filled correctly. The unit tests pass, but I am also checking the full end-to-end test. I will let you know how it goes...

I can confirm that this change does not affect the PGE output in a full test.

ctgh commented 4 months ago

Thanks all for your reviews!