Skip to content

Conversation

@TYTY666
Copy link

@TYTY666 TYTY666 commented Apr 28, 2024

Dear gnssanalysis developers, hello! Thank you for your work.

We noticed that there seems to be some errors in the function used to calculate SISRE in the gn_diffaux.py file of gnssanalysis. We believe that when calculating the SISRE indicator, clk_diff should be multiplied by the speed of light and mutually offset with the radial orbit deviation.

We have corrected this issue. If you think our correction is correct, please accept our PR. If there are any questions, please give us feedback.

@ronaldmaj
Copy link
Collaborator

Hi TYTY666,

Thanks so much for your PR. It's a really good catch and something we missed when creating these functions.

I have put through a parallel PR in order to move the multiplication by the speed of light up to where the clk_diff variable is defined. This aligns better with how we currently calculate the clk values in the diffclk function

The one area we will still take some time to investigate is the minus sign you've introduced in alpha * radial - clk_diff. This could very well be correct but we need to double check this and verify that this is the way it should be.

We will comment here again on this PR if we do find something.

Thanks again for bringing our attention to this :)

@seballgeyer
Copy link
Collaborator

Hello
Thanks for your PR.
After review, we came to the conclusion that the original version of the code is correct (for the moment).

  1. the CLIGHT factor has been added when calling the compare_clk function.
  2. The change of sign is not necessary as the differencing of orbits is doing (orb_B - orb_A) but differencing of clocks is doing (clk_A - clk_B).

We will work in unifying the definition of the differentiation functions and adjust your PR later (removing the CLIGHT factor).

Copy link
Collaborator

@seballgeyer seballgeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for other fixes before merging this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants