SuperDARN / rst

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

Modifying fitacf 2.5, fitex2, and lmfit libraries to correctly handle multi-channel tdiff values #494

Closed egthomas closed 2 years ago

egthomas commented 2 years ago

This pull request addresses the issue identified by @pasha-ponomarenko in #493 where channel B tdiff values were not being correctly assigned when using make_fit with FITACF 2.5. I had to move the channel-dependent tdiff assignment from FitACFMake (which is only called once by make_fit) to fill_fit_block (which is called for every record). I've also modified the fitex2 and lmfit code to assign the channel-dependent tdiff values correctly, even though those fitting algorithms do not calculate elevation angles.

One potential issue I've identified is that if a radar's other hardware information (eg bmsep, bmoff, etc) change partway through a rawacf file (or input file stream) then that may not be accurately captured in the output fitacf-format file. I don't know if that ever happens, since a new rawacf file is typically created whenever a radar stops and then starts again, but it may be worth investigating in the future.

pasha-ponomarenko commented 2 years ago

@egthomas , thanks a lot for the timely response! I downloaded and installed this branch and run the same tests. I did make sure that I run the appropriate version of RST. Unfortunately, from this side it looks like the problem with FITACF2.5 still persists: changing tdiff[0] leads to changing in elevation in both channels and changing tdiff[1] has no effects on either channel.

egthomas commented 2 years ago

@pasha-ponomarenko are you sure you're modifying the correct line of the hdw.dat.han file? Note that in the new format, it would be the middle line that needs modification, and not the final line with a -1 status flag.

I just ran a few of my own tests using that file (20170319.1001.00.han.rawacf) and am seeing different elevation values on channel B with time_plot when tdiff[1] is 0.181 ns (the default) vs when I manually set it to zero. Setting tdiff[0] to zero is not having any impact on the plotted elevation values from channel B. I also added some fprintf statements to stderr inside the elevation_v2 function and am seeing the correct tdiff values being printed for each channel.

pasha-ponomarenko commented 2 years ago

@egthomas , yes, I am changing the correct HDW file in rst-fix_stereo_tdiff, but I will of course triple-check -- I might have done something wrong somewhere.
:-o

pasha-ponomarenko commented 2 years ago

@egthomas , just to be on the safe side, I rebooted the computer, and now everything works as intended. :-) Apologies for the false alarm! :-( I am not sure what was happening as I made sure that I used the correct RST version in a particular command window through "echo $RSTPATH". :-\ From now on I will reboot any time I install a new RST version...

pasha-ponomarenko commented 2 years ago

I guess I should merge it, right?

egthomas commented 2 years ago

@pasha-ponomarenko glad you were able to get it working, thanks! Maybe let's leave it at least through the weekend in case someone else wants to test it next week.

pasha-ponomarenko commented 2 years ago

@egthomas , no problem.

ecbland commented 2 years ago

I've done a quick test with some LYR data and running into a similar problem to @pasha-ponomarenko.

Using a fresh install of this branch, I ran make_fit 20180323.0801.00.lyr.rawacf with tdiff modified on each channel:

## TDIFF=0.000 on both channels (original values)
dmapdump -d orig.fitacf2 | grep -A 1 '"elv"' | head -5
    float   "elv" [10]=
        0.000000,   15.179274,  18.442736,  19.126097,  19.576908,  19.990540,  19.115797,  13.413126,  0.000000,   0.000000
--
    float   "elv" [7]=
        19.747654,  22.608921,  20.259226,  20.081120,  22.701605,  19.072580,  19.619280

## Ch1 TDIFF=0.100    Ch2 TDIFF=0.000  (BOTH CHANNELS HAVE DIFFERENT ELEVATIONS)
dmapdump -d ch1mod.fitacf2 | grep -A 1 '"elv"' | head -5
    float   "elv" [10]=
        0.000000,   14.648138,  17.988134,  18.684719,  19.143826,  19.564789,  18.674225,  12.828156,  0.000000,   0.000000
--
    float   "elv" [7]=
        19.319868,  22.225563,  19.840170,  19.659065,  22.319525,  18.632666,  19.189241

## Ch1 TDIFF=0.000    Ch2 TDIFF=0.100   (HAS NO EFFECT)
dmapdump -d ch2mod.fitacf2 | grep -A 1 '"elv"' | head -5
    float   "elv" [10]=
        0.000000,   15.179274,  18.442736,  19.126097,  19.576908,  19.990540,  19.115797,  13.413126,  0.000000,   0.000000
--
    float   "elv" [7]=
        19.747654,  22.608921,  20.259226,  20.081120,  22.701605,  19.072580,  19.619280

Would someone else mind checking this? Something strange is going on.


@egthomas We should merge this into the release branch since it's a bugfix. I'll update that now.

egthomas commented 2 years ago

@ecbland thanks for testing - this is really surprising, because I've added print statements to both the FitACF and elevation_v2 functions, eg

    goose = (prm->stid == GOOSEBAY);
    fprintf(stderr,"input->prm.channel: %d, input->prm.tdiff: %lf\n",input->prm.channel,input->prm.tdiff);
    fnum = do_fit(input, 5, goose, fit->rng, fit->xrng, fit->elv, &fit->noise);

and am seeing the correct tdiff values being assigned to each channel, eg

input->prm.channel: 1, input->prm.tdiff: 0.100000
input->prm.channel: 2, input->prm.tdiff: 0.000000

but am seeing the same behavior where the channel 2 elevation angles change even when only the channel 1 tdiff is modified. I'll keep digging and let you know what I find.

pasha-ponomarenko commented 2 years ago

@egthomas , @ecbland , now I am surprised as I do not see any anomalous behavior using fix_stereo_tdiff. I have checked the elevation numbers using my IDL code based on RST DLL's, and they look as expected: data for given channels change only when the respective tdiff's change.

pasha-ponomarenko commented 2 years ago

@ecbland , with my Linux ignorance (illiteracy?) I may be missing something about your example: how do you separate different channels while applying grep to dmapdump output? I would like to run this on my sample dataset.

pasha-ponomarenko commented 2 years ago

I guess you just utilise the fact that for the stereo mode the channels are interleaved in time domain. :-)

OK then, here are the results:

 tdiff=[0.,0.] Zero tdiffs for reference
ponomarenko@pasha-linux-main:~/data/raw/temp> dmapdump -d probe00.fitacf | grep -A 1 '"elv"' | head -5
        float   "elv" [15]=
                19.753082,      24.549498,      16.476673,      15.517507,      11.998155,      9.802762,       13.776604,      0.000000,       13.700253,      14.264645,      12.752331,      12.012776,      7.446674,       7.595893,       11.700606
--
        float   "elv" [15]=
                14.519813,      0.000000,       19.625330,      0.000000,       13.296954,      0.000000,       19.064928,      21.001432,      0.000000,       10.545556,      27.754568,      0.000000,       22.664364,      20.119640,      0.000000
 tdiff=[0.135,0.] Non-zero tdiff[0] leads to changes in channel 1 and no changes in channel 2       
ponomarenko@pasha-linux-main:~/data/raw/temp> dmapdump -d probe10.fitacf | grep -A 1 '"elv"' | head -5
        float   "elv" [15]=
                25.288696,      9.480069,       22.790430,      22.096588,      19.736933,      18.447868,      20.889597,      0.000000,       20.838348,      21.220667,      20.214937,      19.746040,      17.263113,      17.331259,      19.553007
--
        float   "elv" [15]=
                14.519813,      0.000000,       19.625330,      0.000000,       13.296954,      0.000000,       19.064928,      21.001432,      0.000000,       10.545556,      27.754568,      0.000000,       22.664364,      20.119640,      0.000000

tdiff=[0.,0.181] Non-zero tdiff[1] leads to changes in channel 2 and no changes in channel 1              
ponomarenko@pasha-linux-main:~/data/raw/temp> dmapdump -d probe01.fitacf | grep -A 1 '"elv"' | head -5
        float   "elv" [15]=
                19.753082,      24.549498,      16.476673,      15.517507,      11.998155,      9.802762,       13.776604,      0.000000,       13.700253,      14.264645,      12.752331,      12.012776,      7.446674,       7.595893,       11.700606
--
        float   "elv" [15]=
                4.241342,       0.000000,       13.589991,      0.000000,       28.712902,      0.000000,       12.789234,      15.469679,      0.000000,       27.466951,      23.727001,      0.000000,       17.620489,      14.277974,      0.000000

tdiff=[0.135,0.181] Non-zero tdiff[0] and tdiff[1] lead to changes in both channel 1 and channel 2
ponomarenko@pasha-linux-main:~/data/raw/temp> dmapdump -d probe11.fitacf | grep -A 1 '"elv"' | head -5
        float   "elv" [15]=
                25.288696,      9.480069,       22.790430,      22.096588,      19.736933,      18.447868,      20.889597,      0.000000,       20.838348,      21.220667,      20.214937,      19.746040,      17.263113,      17.331259,      19.553007
--
        float   "elv" [15]=
                4.241342,       0.000000,       13.589991,      0.000000,       28.712902,      0.000000,       12.789234,      15.469679,      0.000000,       27.466951,      23.727001,      0.000000,       17.620489,      14.277974,      0.000000
ecbland commented 2 years ago

@pasha-ponomarenko Did you use fitacf2.5 or fitacf3.0 for your most recent testing?

pasha-ponomarenko commented 2 years ago

@ecbland , FITACF2.5:

ponomarenko@pasha-linux-main:~/data/raw/temp> dmapdump -d probe11.fitacf | head 
scalars:
        char    "radar.revision.major" = 1
        char    "radar.revision.minor" = 12
        char    "origin.code" = 1
        string  "origin.time" = "Mon Mar 28 16:21:54 2022"
        string  "origin.command" = "make_fit -fitacf-version 2.5 20170319.1001.00.han.rawacf"
        short   "cp" = 153
        short   "stid" = 10
        short   "time.yr" = 2017
        short   "time.mo" = 3
pasha-ponomarenko commented 2 years ago

It is fishy indeed: I processed the same file as you, @ecbland, and replicated your results with tdiff=0.100:

ponomarenko@pasha-linux-main:~/data/raw/temp> dmapdump -d probe00_lyr.fitacf | grep -A 1 '"elv"' | head -5
        float   "elv" [10]=
                0.000000,       15.179274,      18.442736,      19.126097,      19.576908,      19.990540,      19.115797,      13.413126,      0.000000,       0.000000
--
        float   "elv" [7]=
                19.747654,      22.608921,      20.259226,      20.081120,      22.701605,      19.072580,      19.619280
ponomarenko@pasha-linux-main:~/data/raw/temp> dmapdump -d probe10_lyr.fitacf | grep -A 1 '"elv"' | head -5
        float   "elv" [10]=
                0.000000,       14.648138,      17.988134,      18.684719,      19.143826,      19.564789,      18.674225,      12.828156,      0.000000,       0.000000
--
        float   "elv" [7]=
                19.319868,      22.225563,      19.840170,      19.659065,      22.319525,      18.632666,      19.189241
ponomarenko@pasha-linux-main:~/data/raw/temp> dmapdump -d probe01_lyr.fitacf | grep -A 1 '"elv"' | head -5
        float   "elv" [10]=
                0.000000,       15.179274,      18.442736,      19.126097,      19.576908,      19.990540,      19.115797,      13.413126,      0.000000,       0.000000
--
        float   "elv" [7]=
                19.747654,      22.608921,      20.259226,      20.081120,      22.701605,      19.072580,      19.619280
ponomarenko@pasha-linux-main:~/data/raw/temp> dmapdump -d probe11_lyr.fitacf | grep -A 1 '"elv"' | head -5
        float   "elv" [10]=
                0.000000,       14.648138,      17.988134,      18.684719,      19.143826,      19.564789,      18.674225,      12.828156,      0.000000,       0.000000
--
        float   "elv" [7]=
                19.319868,      22.225563,      19.840170,      19.659065,      22.319525,      18.632666,      19.189241

Furthermore, I obtained qualitatively the same result for tdiff=0.03:


ponomarenko@pasha-linux-main:~/data/raw/temp> dmapdump -d probe00_lyr.fitacf | grep -A 1 '"elv"' | head -5
        float   "elv" [10]=
                0.000000,       15.179274,      18.442736,      19.126097,      19.576908,      19.990540,      19.115797,      13.413126,      0.000000,       0.000000
--
        float   "elv" [7]=
                19.747654,      22.608921,      20.259226,      20.081120,      22.701605,      19.072580,      19.619280
ponomarenko@pasha-linux-main:~/data/raw/temp> dmapdump -d probe10_lyr.fitacf | grep -A 1 '"elv"' | head -5
        float   "elv" [10]=
                0.000000,       26.165209,      28.369848,      28.853714,      29.176729,      29.475676,      28.846369,      25.055233,      0.000000,       0.000000
--
        float   "elv" [7]=
                29.515221,      31.637815,      29.886717,      29.756960,      31.708300,      29.030758,      29.422581
ponomarenko@pasha-linux-main:~/data/raw/temp> dmapdump -d probe01_lyr.fitacf | grep -A 1 '"elv"' | head -5
        float   "elv" [10]=
                0.000000,       15.179274,      18.442736,      19.126097,      19.576908,      19.990540,      19.115797,      13.413126,      0.000000,       0.000000
--
        float   "elv" [7]=
                19.747654,      22.608921,      20.259226,      20.081120,      22.701605,      19.072580,      19.619280
ponomarenko@pasha-linux-main:~/data/raw/temp> dmapdump -d probe11_lyr.fitacf | grep -A 1 '"elv"' | head -5
        float   "elv" [10]=
                0.000000,       26.165209,      28.369848,      28.853714,      29.176729,      29.475676,      28.846369,      25.055233,      0.000000,       0.000000
--
        float   "elv" [7]=
                29.515221,      31.637815,      29.886717,      29.756960,      31.708300,      29.030758,      29.422581

So, apparently, for HAN is works as for a stereo radar but treats LYR as a mono one.

egthomas commented 2 years ago

So, apparently, for HAN is works as for a stereo radar but treats LYR as a mono one.

After reading this comment I thought maybe LYR was not recording an offset value in the radar parameter block, but that is not the case. I also thought maybe the "random" tdiff values we are choosing are near the 2pi ambiguity for LYR, but I don't think that would explain the behavior either. I suppose this should be tested at the other Stereo radars as well (eg PYK, HAN, UNW).

egthomas commented 2 years ago

This might not solve the problem being considered in this pull request / issue, but maybe we should add the tdiff value used for each record to fitacf-format files?

pasha-ponomarenko commented 2 years ago

Hopefully, here are some good news! :-) When I looked at the elevation range-time maps I realised that the code works as expected even for LYR. I dumped all data in an ASCII file and found that the problem is that dmapdumpdoes not dump "empty" records. These are two first records from channel 1:

"elv" [10]=
        0.000000,   15.179274,  18.442736,  19.126097,  19.576908,  19.990540,  19.115797,  13.413126,  0.000000,   0.000000

"elv" [7]=
        19.747654,  22.608921,  20.259226,  20.081120,  22.701605,  19.072580,  19.619280

and the channel 2 records in between those two are dropped because all range gates are "bad".

pasha-ponomarenko commented 2 years ago

So, beware of dmapdump bearing false alarms!

pasha-ponomarenko commented 2 years ago

This might not solve the problem being considered in this pull request / issue, but maybe we should add the tdiff value used for each record to fitacf-format files?

@egthomas, not a bad idea!

egthomas commented 2 years ago

@pasha-ponomarenko you are correct, thank you! The first channel 2 record with any fitted data does not occur until several seconds into the file, so the first few results shown by the grep / head command all belong to channel 1.

ecbland commented 2 years ago

Thanks @egthomas and @pasha-ponomarenko for figuring out this unexpected behaviour and sorry for sending you down the rabbit hole. I've tested again with another file and it works as expected. I guess we are ready to merge now?

egthomas commented 2 years ago

@ecbland yes, I believe so. Thank you for also reviewing these changes!