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

Adds valid height check for DEM intersection #121

Merged
merged 6 commits into from
Feb 29, 2024
Merged

Conversation

jlaura
Copy link
Collaborator

@jlaura jlaura commented Feb 28, 2024

Licensing

This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words:

This work is free and unencumbered software released into the public domain. In jurisdictions that recognize copyright laws, the author or authors of this software dedicate any and all copyright interest in the software to the public domain.

  • I dedicate any and all copyright interest in this software to the public domain. I make this dedication for the benefit of the public at large and to the detriment of my heirs and successors. I intend this dedication to be an overt act of relinquishment in perpetuity of all present and future rights to this software under copyright law.

@jlaura
Copy link
Collaborator Author

jlaura commented Feb 29, 2024

@acpaquette I just realized that this repo has not testing CI 😨!! This PR is passing locally (with warnings related to the PVL library). How do you want to proceed? I don't want this to become an uber PR. I can open another PR with testing setup, we can get that merged, and then re-review/re-run this one. Or you could pull this PR local, also verify that it is passing and then we can get an issue open.

I did not mean for this to become a thing!

Copy link
Collaborator

@acpaquette acpaquette left a comment

Choose a reason for hiding this comment

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

Couple comments to address, otherwise PR looks good!

tests/test_csm.py Outdated Show resolved Hide resolved
tests/test_csm.py Outdated Show resolved Hide resolved
tests/test_csm.py Outdated Show resolved Hide resolved
@mock.patch.object(csm, 'get_radii', return_value=(10,10))
@mock.patch('pyproj.transformer.Transformer.transform', return_value=(0,0,0))
@mock.patch.object(csm, '_compute_intersection_distance', return_value=0)
def test_generate_ground_point_with_dtm(mock_sensor, pt, _):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I can tell, pytest is counting the number of decorators and then assuming that n-decorators == n-args to the func. To get the test parser to work, I added the _ arg. This seems super janky. Any thoughts on whether this approach should be different?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure here, you might try putting the other mock.patch.object above the mock.patch. Some of pythons test suite is very order dependent

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comment was 💰. The mock.patch.object needs to be passed into the test as an arg. The order is also important where the mocked stuff needs to be first in the test signature, then any fixtures.

Local results with this setup: 5 passed, 4 warnings in 1.89s

@jlaura jlaura merged commit ca104e7 into DOI-USGS:main Feb 29, 2024
1 check passed
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.

2 participants