ACHMartin / seastar_project

4 stars 0 forks source link

Update to current direction #265

Closed DavidMcCann-NOC closed 1 year ago

DavidMcCann-NOC commented 1 year ago

Change to current direction computation that corrects issues with wrongly computed current vectors.

With the interferogram sign convention flipped, this change now correctly computes the retrieved current direction on all days of the Iroise Sea campaign, including the 17th (without any differential treatment of the beam conventions)

It it now known that the differential treatment of beam direction convention on the 17th effectively compensated for this error in the vector direction computation, thanks to the geophysical structures (current jets) used to visually verify the computed current direction being almost exactly parallel with either the fore or aft beam. Therefore by adjusting a beam direction convention the orthogonal current components were in the right magnitudes for the old version of this function to produce correct current directions.

However on subsequent flights, where a differential beam treatment was not applied, these current directions were incorrect by +90 degrees. This was not fully noticed as the data on the 22nd was, until now, primarily used in the star pattern analysis which does not make use of this function

With this change to compute_current_magnitude_and_direction(), combined with the phase inversion performed during L1B processing (for all beams), current vectors are now correctly computed on all days

ACHMartin commented 1 year ago

Hi @elemerle,

@DavidMcCann-NOC found a bug in the code, and it seems to work fine now. Given it is a critical part of the processing chain, could you please double-check the calculation in compute_current_magnitude_and_direction(level1, level2).

We should implement an unit test for this function, to test the specific bug and direction. Given the pressure, I am not sure it is the best moment to spend time on it, BUT if one of you did some test (a la mano), either implement it as a unit test, or store the test in github by opening a new issue (anyway we need to open a new issue to implement a unit test on this in the future given how critical this function is).

ACHMartin commented 1 year ago

@DavidMcCann-NOC I notice you didn't change the _version.py file. Please change it just before merging it. Ideally, github should prevent us merging it, if we don't touch this file

ACHMartin commented 1 year ago

@elemerle, @DavidMcCann-NOC for the direction I don't understand why we don't have the 90deg any more between the trigonometric convention (0 in math at 90deg in meteorological or oceano convention)

elemerle commented 1 year ago

@ACHMartin Because for me u and v are already in oceanographic convention since the azimuth of the Antenna is from North