search

SuperDARN / rst

Radar Software Toolkit (RST)
https://superdarn.github.io/rst/
GNU General Public License v3.0
22 stars 17 forks source link

Fitacf 3.0 Elevation phi sign correction bug #189

Closed mts299 closed 5 years ago

mts299 commented 5 years ago

Bill pointed out the following issue in Fitacf 3.0

I've been using the version of fitacf 3 that is included in rst-4.1 and am for the most part pleased with the results, however...

I started looking at elevation angles at Kodiak since going to the USRP system and think I see a problem. If you look at the attached plot you will see elevation angles at 0340 UT on 20180924. The near range stuff is ionospheric, while the far range stuff is ground scatter. It looks like the ionospheric scatter is coming from behind the radar, while the ground scatter looks like it's coming from the front. However, I believe the opposite is the case (ionosphere in front, ground in back).

If I analyze the data with fitacf-2.5 and set the phidiff parameter in the hdw.dat.kod file, I get the behavior I expect, while the parameter has no effect in fitacf-3.0.

I can fix fitacf-3.0 with the two following changes to determinations.c:

adding the following at line 393 fixes the elevation angles phi_sign*=fit_prms->phidiff;

and phi0 can be fixed by changing line 464 to look like this: fit_range_array[range_node->range].phi0 = atan2(imag,real)*fit_prms->phidiff;

Please let me know if you agree with this change.

Bill

@pasha-ponomarenko noticed this was also an issue in RST 4.2 as well.

mts299 commented 5 years ago

Here are some tests I did with the develop branch: make_fit -fitacf-version 3.0 20180624.0001.00.sas.rawacf > normal_develop.sas.fitacf make_fit -fitacf-version 3.0 20180624.0001.00.sas.rawacf > reveresed_develop.sas.fitacf time_plot -e -png -st 00:00 -et 02:00 normal_develop.sas.fitacf > normal_develop.sas.png time_plot -e -png -st 00:00 -et 02:00 reversed_develop.sas.fitacf > reversed_develop.sas.png

Normal phi sign normal_develop sas

Reversed phi sign reversed_develop sas

egthomas commented 5 years ago

@mts299 @pasha-ponomarenko should those two time series plots look identical? Saskatoon has a phidiff of 1 so multiplying phi0 by it won't make any difference...

pasha-ponomarenko commented 5 years ago

@egthomas, Marina has changed the phidiff sign in the hdw file for the second plot.

mts299 commented 5 years ago

@egthomas Sorry I forgot to mention I flipped the phase sign in the hdw file to test the behavior. So before making make_fit -fitacf-version 3.0 20180624.0001.00.sas.rawacf > reveresed_develop.sas.fitacf I editted the saskatoon hardware file to have a phase sign of -1.

pasha-ponomarenko commented 5 years ago

I believe the above plots are supposed to illustrate that the phidiff correction does not work in the current version of FITACF3.0.

pasha-ponomarenko commented 5 years ago

Here are a bit more details on this issue. Bill Bristow has identified a problem with FITACF3 in RST4.1. He needed to reverse the interferometer phase in order to correct recent Kodiak data so he attempted to use phidiff parameter from the hardware file. This parameter, which can be set to either 1 or -1, has been introduced ages ago in order to account for unintentional swapping cables in the intrferometer phasing matrix. By reversing phidiff sign in the respective hardware file Bill achieved the expected change for files processed by FITACF2.5, however, the output for FITACF3 remained unchanged. After a closer look at the code I realised that in converting FITACF2.5 into FITACF3 we failed to replicate this particular functionality. The extensive testing performed on FITACF3 has not included data which would require such correction (the last occasion goes back to1992!) so that is why the error has gone unspotted for a couple of years. The fix is simple: parameter phi0 (lag 0 cross-phase estimate) has to be multiplied by phidiff before it is used in the elevation angle routine.

While analysing this particular issue, I also stumbled across yet another related mistake. In the “old” elevation procedure the correction for the difference in the altitudes of the arrays (non-zero offset in Z direction) is based on calculating an arcsine of the ratio (Z/sqrt(X^2+Y^2+Z^2)). In the code, this parameter is multiplied by phidiff, which is simply wrong. I guess that this is just a leftover from the previous attempts to account for the phase reversal which should have been removed after applying the above fix to phi0.

mts299 commented 5 years ago

@egthomas you can see the differences in my PR #190

egthomas commented 5 years ago

@mts299 ok yes, the figures do appear different in the pull request. Here in the issue they look the same to me though.

mts299 commented 5 years ago

@egthomas that is the bug... they are not supposed to look the same. So once we applied the fixes done in the PR #190 then you can see that the plots are different after making a phase change.

I could add the develop branch test to the PR #190 if that makes it more clear?

egthomas commented 5 years ago

@mts299 sorry, I didn't catch that the above figures were strictly with the develop branch. My mistake!

mts299 commented 5 years ago

@egthomas No worries, I was trying to show the issue in the issue by assuming we had not made a branch yet to fix it then made a PR to show we fixed with the phidiff_fix branch :)

If we are all caught up can I close this issue?

pasha-ponomarenko commented 5 years ago

By the way, after contemlating this issue for a while I started to think that the more universal fix would be to apply the correction directly to XCF (i.e. to multiply the imaginary part by phidiff) before or while calcualting cross-phase. In this case we cover two ways of calculating elevation: (1) directly from "unfitted" lag 0 x-phase and (2) from fitting a linear funciton to x-phase. Both ways are currently implemented in FITACF3.0 for testing purposes with the results stored in fit.elv and fit.elv_high, respectively.

mts299 commented 5 years ago

@pasha-ponomarenko I am going to add this into PR #190 where the solution is given.

Closing this issue. Resolved in PR #190

sshepherd commented 5 years ago

I am wondering if someone should first contact Ray or someone else from that generation before considering this issue closed. Recall that the "phase sign" flag was introduced by Ray to account for the fact that he likely left the Goose Bay radar with cables connected in the wrong order. I am not 100% certain what those cables were. They (Ray, Kile, etc.) obviously had a fix in mind and implemented it in the code. Bill has used this flag to account for a phase flip in the Kodiak and McMurdo radars when moving to USRPs. The fix works, but is it the same situation that existed at Goose Bay for which the flag was introduced? I don't know. I am not sure data from that time-period exists (1987-1992) so perhaps my point doesn't matter and we should just use that flag for a phase sign flip.

Changing the elevation code to fix a "bug" also seems to violate one of the tenants of keeping fitACF2.5 the same, i.e., for reproducibility.

On Mon, Oct 15, 2018 at 1:04 PM mts299 notifications@github.com wrote:

Closed #189 https://github.com/SuperDARN/rst/issues/189.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/SuperDARN/rst/issues/189#event-1904680931, or mute the thread https://github.com/notifications/unsubscribe-auth/AC2DPxeSRncr5w4z7oKZHppSwq52PwFHks5ulL_-gaJpZM4XcoFA .

pasha-ponomarenko commented 5 years ago

Changing the elevation code to fix a "bug" also seems to violate one of the tenants of keeping fitACF2.5 the same, i.e., for reproducibility.

You are talking about Bill's approach, but in this pull request we change it before the elevation code, in full accordance with what's done in FITACF2.

sshepherd commented 5 years ago

Yes, I stand corrected. Keep using the old elevation code.

On Mon, Oct 15, 2018 at 3:59 PM pasha-ponomarenko notifications@github.com wrote:

Changing the elevation code to fix a "bug" also seems to violate one of the tenants of keeping fitACF2.5 the same, i.e., for reproducibility.

You are talking about Bill's approach, but in this pull request we change it before the elevation code, in full accordance with what's done in FITACF2.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/SuperDARN/rst/issues/189#issuecomment-429992107, or mute the thread https://github.com/notifications/unsubscribe-auth/AC2DPyD5NBZQB0Xjl1xNlWcciAuoJn14ks5ulOkagaJpZM4XcoFA .

egthomas commented 5 years ago

I am not sure data from that time-period exists (1987-1992) so perhaps my point doesn't matter and we should just use that flag for a phase sign flip.

As of the 2018 SD Workshop, Gareth Chisham indicated that looking for these data at BAS was still on his to-do list. I can try following up with him again.