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

gnss: handle longitude <=-180 or >=360 #1290

Merged
merged 2 commits into from
Nov 11, 2024
Merged

gnss: handle longitude <=-180 or >=360 #1290

merged 2 commits into from
Nov 11, 2024

Conversation

yunjunz
Copy link
Member

@yunjunz yunjunz commented Nov 11, 2024

Description of proposed changes

  • utils/utils0.py: add standardize_longitude() to handle the longitude data range

    • ensure longitude is within (-180, 360)
    • could specify the data range format, in either [0, 360) or (-180, 180] via lon_range arg
  • objects/gnss.py: call ut.standardize_longitude() in all the child class of GNSS

This error occurs for the GNSS_UNR sites in Japan, where the longitude of G073 can be -229. In the old code, this results in a NaN value for inc/az_angle extraction from the geometry file.

Reminders

  • Pass Pre-commit check (green)
  • Pass Codacy code review (green)
  • Pass Circle CI test (green)
  • Make sure that your code follows our style. Use the other functions/files as a basis.
  • If modifying functionality, describe changes to function behavior and arguments in a comment below the function declaration.

Summary by Sourcery

Ensure longitude values are correctly handled within the GNSS module by introducing a function to set longitude ranges, preventing errors in data processing.

Bug Fixes:

  • Fix longitude handling for GNSS_UNR sites to prevent NaN values by ensuring longitude is within the valid range.

Enhancements:

  • Introduce a new function _set_longitude_range() to standardize longitude values across GNSS data processing.

+ objects/gnss.py: add `_set_longitude_range()` to handle the longitude data range
   - ensure longitude is within (-180, 360)
   - could specify the data range format, in either [0, 360) or (-180, 180] via `lon_range` arg

+ call `_set_longitude_range()` in all the child class of GNSS

This error is happending for the GNSS_UNR sites in Japan, where the longitude of G073 can be -229, resulting in NaN value for inc/az_angle extration from the geometry file, in the old code.
Copy link

sourcery-ai bot commented Nov 11, 2024

Reviewer's Guide by Sourcery

This PR introduces a new longitude handling mechanism in the GNSS module to properly handle longitude values that fall outside the standard ranges. The implementation adds a new utility function _set_longitude_range() that normalizes longitude values to either (-180, 180] or [0, 360) ranges, and updates all GNSS child classes to use this function.

Class diagram for GNSS longitude handling update

classDiagram
    class GNSS {
        +_set_longitude_range(lon, lon_range)
    }
    class GNSSChild1 {
        +get_site_lat_lon(print_msg)
    }
    class GNSSChild2 {
        +get_site_lat_lon(print_msg)
    }
    class GNSSChild3 {
        +get_site_lat_lon(print_msg)
    }
    GNSS <|-- GNSSChild1
    GNSS <|-- GNSSChild2
    GNSS <|-- GNSSChild3
    note for GNSS "New method _set_longitude_range added to handle longitude normalization"
    note for GNSSChild1 "Updated to use _set_longitude_range for longitude normalization"
    note for GNSSChild2 "Updated to use _set_longitude_range for longitude normalization"
    note for GNSSChild3 "Updated to use _set_longitude_range for longitude normalization"
Loading

File-Level Changes

Change Details Files
Added new longitude normalization utility function
  • Implements longitude range normalization to either (-180, 180] or [0, 360)
  • Handles both scalar and numpy array inputs
  • Ensures input values are first normalized to (-180, 360) range before final range conversion
src/mintpy/objects/gnss.py
Updated longitude handling across GNSS classes
  • Replaced direct longitude adjustments with calls to _set_longitude_range()
  • Standardized longitude range to (-180, 180] across all GNSS child classes
  • Added longitude normalization after loading site coordinates
src/mintpy/objects/gnss.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

codeautopilot bot commented Nov 11, 2024

PR summary

This Pull Request addresses a bug in the GNSS module related to handling longitude values that fall outside the typical range, specifically for GNSS_UNR sites in Japan. The changes introduce a new function, standardize_longitude(), which ensures longitude values are standardized within a specified range, either (-180, 180] or [0, 360). This prevents errors such as NaN values during data processing, particularly in the inc/az_angle extraction from geometry files.

Suggestion

Consider adding unit tests for the standardize_longitude() function to ensure its robustness and correctness across various input scenarios. This will help maintain code quality and prevent future regressions. Additionally, updating the documentation to reflect these changes and the new function's usage would be beneficial for users and developers.

Disclaimer: This comment was entirely generated using AI. Be aware that the information provided may be incorrect.

Current plan usage: 0.00%

Have feedback or need help?
Discord
Documentation
[email protected]

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @yunjunz - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

src/mintpy/objects/gnss.py Outdated Show resolved Hide resolved
src/mintpy/objects/gnss.py Outdated Show resolved Hide resolved
+ rename `_set_longitude_range()` to `standardize_longitude()`

+ move the function from objects/gnss.py to utils/utils0.py
@yunjunz yunjunz merged commit c7e77cc into insarlab:main Nov 11, 2024
7 checks passed
@yunjunz yunjunz deleted the gnss branch November 11, 2024 10:48
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.

1 participant