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.

Sign up for GitHub

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

Jump to bottom

Feature #3012 ugrid for 3D points (PointXYZ) #3035

Merged
merged 13 commits into from
Dec 13, 2024
Merged

Feature #3012 ugrid for 3D points (PointXYZ) #3035

merged 13 commits into from
Dec 13, 2024

Conversation

hsoh-u
Copy link
Collaborator

@hsoh-u hsoh-u commented Dec 11, 2024

Expected Differences

Added two test utilities for 3D KDTree. The lat/lon in degree is converted to km as ECEF (Earth Centered Earth Fixed). KDTree was tested with PointXYZ

  • Do these changes introduce new tools, command line arguments, or configuration file options? [Yes]

    If yes, please describe:

Added two test utilities at internal/test_util/libcode/vx_grid

  • search_3d_kdtree: standalone KDTree for 3 dimensional point data (lat/lon/hgh)

    • read NetCDF input
    • build KDTree with 3D point data
    • search the nearest point from user input

  • search_3d_kdtree_api: The KDTree for 3 dimensional point data (lat/lon/hgh) is implemented at src/libcode/vx_grid

    • do the same thing with search_3d_kdtree
    • unit test for llh_to_ecef (lat/lon/height or altitude to Earth Centered Earth Fixed in km)

  • Do these changes modify the structure of existing or add new output data types (e.g. statistic line types or NetCDF variables)? [No]

    If yes, please describe:

Pull Request Testing

  • Describe testing already performed for these changes:

The build is ready at seneca (/d1/personal/hsoh/git/pull_request/MET_feature_3012_ugrid_3D/internal/test_util/libcode/vx_grid):

The same arguments for search_3d_kdtree and search_3d_kdtree_api :

Usage: ./search_3d_kdtree_api netcdf_name lat_deg lon_deg alt_meters <lat_var_name> <lon_var_name> <alt_var_name>
             def_lat_var_name=glat, def_lon_var_name=glon, def_alt_var_name=zalt

./search_3d_kdtree /d1/personal/dadriaan/projects/NRL/PyIRI/pyiri_f4_2020.nc -23.114 334.77 100.
./search_3d_kdtree_api /d1/personal/dadriaan/projects/NRL/PyIRI/pyiri_f4_2020.nc 34.0522 -118.40806 0. glat glon zalt

Unit test for llh_to_ecef:

./search_3d_kdtree

LA:   (34.0522, -118.408, 0) => (-2516.72, -4653, 3551.25) Diff: (0.000361141, 8.08895e-05, -0.00035929)
First Point:   (-9.59874, 287.326, 100) => (1873.07, -6004.14, -1056.53) Diff: (0.000460507, -0.000282083, -9.01243e-05)
Middle Point:   (-27.9711, 107.326, 100) => (-1678.84, 5381.52, -2973.72) Diff: (-0.000100256, -0.000346392, 6.33836e-06)
Last Point:   (75.6264, 287.326, 100) => (473.024, -1516.28, 6156.59) Diff: (-0.000386969, -0.000322839, 0.000451235)

Note: Distance and Diff in km

 /d1/personal/hsoh/git/pull_request/MET_feature_3012_ugrid_3D/internal/test_util/libcode/vx_grid/search_3d_kdtree_api

           LA:   (34.0522, -118.408, 0) => (-2516.72, -4653, 3551.25) Diff: (0.000361141, 8.08895e-05, -0.00035929)
  First Point:   (-9.59874, 287.326, 100) => (1873.07, -6004.14, -1056.53) Diff: (0.000460507, -0.000282083, -9.01243e-05)
 Middle Point:   (-27.9711, 107.326, 100) => (-1678.84, 5381.52, -2973.72) Diff: (-0.000100256, -0.000346392, 6.33836e-06)
   Last Point:   (75.6264, 287.326, 100) => (473.024, -1516.28, 6156.59) Diff: (-0.000386969, -0.000322839, 0.000451235)
  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

Test with other input if available

  • Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [No]

  • Do these changes include sufficient testing updates? [No]

  • Will this PR result in changes to the MET test suite? [No]

    If yes, describe the new output and/or changes to the existing output:

  • Will this PR result in changes to existing METplus Use Cases? [No]

    If yes, create a new Update Truth METplus issue to describe them.

  • Do these changes introduce new SonarQube findings? [No]

    If yes, please describe:

  • Please complete this pull request review by [Fill in date].

Pull Request Checklist

See the METplus Workflow for details.

  • Review the source issue metadata (required labels, projects, and milestone).
  • Complete the PR definition above.
  • Ensure the PR title matches the feature or bugfix branch name.
  • Define the PR metadata, as permissions allow.
    Select: Reviewer(s) and Development issue
    Select: Milestone as the version that will include these changes
    Select: Coordinated METplus-X.Y Support project for bugfix releases or MET-X.Y.Z Development project for official releases
  • After submitting the PR, select the ⚙️ icon in the Development section of the right hand sidebar. Search for the issue that this PR will close and select it, if it is not already selected.
  • After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge.
  • Close the linked issue and delete your feature or bugfix branch from GitHub.

@hsoh-u hsoh-u added this to the MET-12.1.0 milestone Dec 11, 2024
@hsoh-u hsoh-u linked an issue Dec 11, 2024 that may be closed by this pull request
Write test utility to search a 3D cloud of points for the nearest neighbor to a specified (lat, lon, elev) location #3012
Open
22 tasks
JohnHalleyGotway
JohnHalleyGotway requested changes Dec 13, 2024
View reviewed changes
Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@hsoh-u while the SonarQube scan did not fail, I did check the results on server and found.

I looked through those 24 and see that several are easily fixed. Please take a look and resolve whichever can be easily done (especially using empty(), removing unused vars, removing commented out code, and so on).

Generally, I'd like with each PR to:

  1. Reduce the Overall number of code smells.
  2. Review the New Code code smells and fix the ones that are easily fixed.

In my opinion, the ones about complexity and the number of nested loops are very difficult to fix. So it's OK to add news ones of those... as long as you reduce easy ones elsewhere to make sure the total number of smells is reduced.

MET Tools Test Account added 2 commits December 13, 2024 18:28
Per #3012, update the .gitignore settings to ignore the newly created…
edeb3fd 
… test executables in the test_util/libcode/vx_grid directory.
Per #3012, changes to the vx_grid library to address SonarQube code s…
2495099 
…mells in New Code and hopefully reduce the Overall number of code smells.
@JohnHalleyGotway JohnHalleyGotway self-requested a review December 13, 2024 20:36
JohnHalleyGotway
JohnHalleyGotway approved these changes Dec 13, 2024
View reviewed changes
Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway left a comment

Choose a reason for hiding this comment

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

I approve of these changes.

Note the following:

  • I updated the .gitignore settings with these changes.
  • I made changes to the vx_grid library to reduce the SonarQube code smells with these changes.
  • I reviewed the code changes and don't see any obvious problems.
  • I tested the code in seneca /d1/projects/MET/MET_pull_requests/met-12.1.0/beta1/MET-feature_3012_ugrid_3D by rerunning the tests listed in the PR and confirmed that they run without error.

I note that there are no testing or documentation updates but that's fine because this is just adding internal test code. We'll update the tests and docs when the functionality is ready to be used in one of the MET applications.

The total number of SonarQube code smells are reduced from 18,253 in the develop branch down to 18,222 for this feature branch.

@hsoh-u hsoh-u merged commit c7e4345 into develop Dec 13, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnHalleyGotway JohnHalleyGotway JohnHalleyGotway approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
MET-12.1.0
2 participants
@hsoh-u @JohnHalleyGotway