search

SuperDARN / rst

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

Fixing bug in trim_iq #566

Closed egthomas closed 1 year ago

egthomas commented 1 year ago

This pull request modifies IQEncode to remove the extra multiplication by 2 causing a bug in trim_iq output data size (#565), and modifying make_raw, make_sim, and sim_real to correctly handle the IQ chnnum parameter. This bug appears to have originated from the development of the Linux/QNX6 ROS, which is no longer part of the RST.

ecbland commented 1 year ago

Thanks for looking into this @egthomas! I will have time to test this early next week.

ecbland commented 1 year ago

Thanks again @egthomas. I've tested this as follows:

  1. On this branch, the output of trim_iq 20181105.2001.00.hkw.iqdat (i.e. without actually trimming the file) should be identical to the input file except for origin.command/origin.time/origin.code. This is almost true, except that trim_iq adds the fields ifmode=-1 and mplgexs=0. I don't see a problem here, but just noting the difference.

  2. sim_real -iq [fitfile] and make_sim -iq now set iq->chnnum=2. Yes

  3. On this branch I can use make_raw and then make_fit to generate sensible-looking fitted data starting from an iqdat file, compared to the same procedure on develop which looks problematic. Example below for develop (left) and fix_iqdat_chnnum (right).

@egthomas Do you think this testing is sufficient?

@pasha-ponomarenko Since we also discussed this behaviour before I opened #565 -- do you have anything to add?

egthomas commented 1 year ago

@ecbland thanks for testing - I think you've done a very thorough job! Note that make_raw also has a -chnnum option for overriding the chnnum value in the iqdat files (likely because of this very issue).

pasha-ponomarenko commented 1 year ago

@ecbland, good job! 👍 I also discovered that for proper conversion one needs to specify the number of channels in make_raw by using -chnnum switch and was going to look into it later, but you beat me to that! 🙂 I will install this branch on my Linux box and check how it works, but I believe your testing is more than sufficient.

@egthomas , thanks a lot for fixing this problem! 👌

ecbland commented 1 year ago

@pasha-ponomarenko Were you planning to test this or should I merge it now?

pasha-ponomarenko commented 1 year ago

@ecbland , I tested iq_trim, and it works. Please go ahead with merging.