Stanford-NavLab / gnss_lib_py

Modular Python tool for parsing, analyzing, and visualizing Global Navigation Satellite Systems (GNSS) data and state estimates
MIT License
116 stars 28 forks source link

rinex_nav: Incorrect time of clock #155

Closed roberttully95 closed 4 months ago

roberttully95 commented 4 months ago

Bug Overview: The clock correction model is being marked as updated for all satellites of a constellation when updated for any satellite from that constellation using rinex_nav.

Bug Description: When a message with a new timestamp is parsed from a RINEX file, all satellites in the constellation that did not have a message at that time are updated with a 'dummy' record. The time of clock is being incorrectly set when dealing with these 'dummy' records such that all satellites are being treated as having an updated clock correction model when any satellite has an updated model.

https://github.com/Stanford-NavLab/gnss_lib_py/blob/c9457b10fd9f45432c5acd6c7e987e032ea24541/gnss_lib_py/parsers/rinex_nav.py#L213-L214

System Information:

betaBison commented 4 months ago

Hi @roberttully95 ! Thanks for taking the time to submit an issue. First to clarify the issue, I made a minimal working example in this colab notebook with this brdc0730.zip data file: https://colab.research.google.com/drive/1fcLRIkPmziE45Tvz97xcqQv7I2gguAVs?usp=sharing

Currently, there's a row of NaN values for any satellite that doesn't have a rinex entry for that timestep. The behavior you'd expect is for that row of NaN's to not exist so that the length of the RinexNav class output always be equal to the number of total entries in the Rinex data file? Is that correct?

That functionality could be achieved on our end by editing how we drop NaN values after the rinex file is loaded with georinex.

roberttully95 commented 4 months ago

Thanks for making the example! Yes, this illustrates the scenario.

I think that expectation is correct. I would expect only data contained in the original rinex file to be populated in the RinexNav object.

I have verified that the issue lies in how the NaN values are dropped. If the rows are dropped after the leap seconds are added to the data frame it works as expected. In the code below, all NaN rows are never dropped because the leap seconds are non-NaN.

https://github.com/Stanford-NavLab/gnss_lib_py/blob/c9457b10fd9f45432c5acd6c7e987e032ea24541/gnss_lib_py/parsers/rinex_nav.py#L207-L211

I have also verified this fixes the observed issue of incorrect clock correction in the resultant satellite states.

Let me know if I should submit a pull request to submit my local fix. Thanks for your assistance!

betaBison commented 4 months ago

I reordered those lines of code in #156. That change will be reflected in our 1.0.2 version of gnss_lib_py after #157 gets merged into main. That merge will hopefully happen at the end of this week, but let us know if this is a blocker for you and you need it sooner.

In the mean time, you can switch to the derek/rinex-nav-removal branch or add these lines to the top of a google colab to use that dev branch

# install dev branch of gnss_lib_py
import os
os.makedirs("/content/lib", exist_ok=True)
%cd /content/lib
!pip install --upgrade pip --quiet --progress-bar off
!git clone https://github.com/Stanford-NavLab/gnss_lib_py.git --quiet
%cd gnss_lib_py
!git checkout derek/rinex-nan-removal
!git pull
!pip install -e . --quiet --progress-bar off
%cd /content
import site
site.main()
betaBison commented 4 months ago

@roberttully95 This fix has been published to pypi in version 1.0.2. Thanks for taking the time to post an issue and please let us know if you have any further problems