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

Validate new apply_matrix() function further. #110

Closed
erikmannerfelt opened this issue Apr 30, 2021 · 1 comment · Fixed by #531
Closed

Validate new apply_matrix() function further. #110

erikmannerfelt opened this issue Apr 30, 2021 · 1 comment · Fixed by #531
Labels
enhancement Feature improvement or request priority Needs to be fixed rapidly test-suite Issue related to testing

Comments

@erikmannerfelt
Copy link
Contributor

As mentioned in #87, this function is quite convoluted and could potentially produce odd results in cases that tests don't yet properly cover.

@adehecq mentioned potential issues that may arise here:

if abs(matrix_diff[matrix_diff != matrix_diff[2, 3]].mean()) < 1e-4:

Would that not cause a problem if the matrix has other elements that are equal to element [2, 3]?
Also, should it not be strictly equal to 0? The 1e-4 value is somewhat arbitrary here...
In the worse case, we're just doing extra work for nothing.

and suggested:

Not the simplest way, but you could (make a copy,) set the unwanted value to 0 and calculate the mean.

It should be validated if this is a problem, and if so, we should try to fix it.

Other stress tests could be:

  • Introduce test cases with many nans
  • Validate the accuracy of sub-pixel shifts.
  • Run a transformation back and forth many times and see if any bias drift happens.
  • Try more extreme transformations (I would argue that 10 degrees of rotation is already extreme, but we could make it worse!)
@erikmannerfelt erikmannerfelt added the enhancement Feature improvement or request label Apr 30, 2021
@erikmannerfelt
Copy link
Contributor Author

erikmannerfelt commented May 7, 2021

It seems like it struggles with many nans. This has to be checked further!

EDIT: Also @rhugonnet , you have my permission to say "I told you so!", but it seems like the mask dilation should be on by default, to not mess up the num-to-nan edges of a DEM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature improvement or request priority Needs to be fixed rapidly test-suite Issue related to testing
Projects
None yet
2 participants