Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing bug in trim_iq #566

Merged
merged 1 commit into from
May 18, 2023
Merged

Fixing bug in trim_iq #566

merged 1 commit into from
May 18, 2023

Conversation

egthomas
Copy link
Member

@egthomas egthomas commented May 3, 2023

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.

… trim_grid output data size, and modifying make_raw, make_sim, and sim_real to correctly handle the IQ chnnum
@egthomas egthomas added the bug label May 3, 2023
@egthomas egthomas requested a review from ecbland May 3, 2023 14:56
@ecbland
Copy link

ecbland commented May 5, 2023

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

@egthomas egthomas changed the title Fixing bug in trim_grid Fixing bug in trim_iq May 5, 2023
@ecbland
Copy link

ecbland commented May 8, 2023

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
Copy link
Member Author

egthomas commented May 8, 2023

@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
Copy link
Contributor

@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
Copy link

ecbland commented May 18, 2023

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

@pasha-ponomarenko
Copy link
Contributor

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

@ecbland ecbland merged commit cbb50ab into develop May 18, 2023
@ecbland ecbland deleted the fix_iqdat_chnnum branch May 18, 2023 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants