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

Performance Issue: Unnecessary Re-conversion of Coordinates in point_within_gca #936

Open
amberchen122 opened this issue Sep 4, 2024 · 0 comments · May be fixed by #937
Open

Performance Issue: Unnecessary Re-conversion of Coordinates in point_within_gca #936

amberchen122 opened this issue Sep 4, 2024 · 0 comments · May be fixed by #937
Assignees
Labels
improvement Improvements on existing features or infrastructure

Comments

@amberchen122
Copy link
Collaborator

Proposed new feature or change:

The current implementation of point_within_gca unnecessarily re-converts lat/lon coordinates from Cartesian coordinates, which negatively impacts performance.

The following lines re-compute lat/lon from Cartesian coordinates:

python

GCRv0_lonlat = np.array(
        _xyz_to_lonlat_rad_no_norm(gca_cart[0][0], gca_cart[0][1], gca_cart[0][2])
    )
GCRv1_lonlat = np.array(
    _xyz_to_lonlat_rad_no_norm(gca_cart[1][0], gca_cart[1][1], gca_cart[1][2])
)

However, Uxarray already stores lat/lon coordinates, which can be passed directly to avoid redundant computation. This approach reduces both time and space complexity while aligning better with the overall architecture.

Proposed Solution:

  • Pass the existing lat/lon coordinates directly to eliminate the unnecessary conversion.
  • Update upstream function calls to point_within_gca
@amberchen122 amberchen122 added the improvement Improvements on existing features or infrastructure label Sep 4, 2024
@amberchen122 amberchen122 self-assigned this Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvements on existing features or infrastructure
Projects
Status: 🏗 In progress
Development

Successfully merging a pull request may close this issue.

1 participant