-
Notifications
You must be signed in to change notification settings - Fork 82
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
Radar and Spinner Lidar Sampling #954
Conversation
Ok, so I'm moving this out of draft as soon as all the checks go through. I think I've addressed everything (albeit in the most minimal way possible). There are a few details to note:
I'd also like to add a reg test for each sampler in a subsequent PR. I can open an issue and assign to myself if that seems worth it. |
return vnew; | ||
} | ||
|
||
void spherical_cap_quadrature( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot of code added in SamplingUtils, and these standalone functions seem like good candidates for unit testing. Don't need tests for every single one, but ideally if there is a basic problem with a known solution for a few of the complicated ones, it would help solidify this part of the code and rule out errors here if there is debugging required in the future.
I think this PR is really close. 2 things come to mind: unit tests for SamplingUtils, which I mentioned above, and the preexisting sampling methodology. Because this PR reworked some of the functions in Sampling.cpp, is there any concern that the changes have affected the ordinary sampling there? Unit tests exist for some of that already, I'm just wanting to confirm that this aspect has been looked at. |
Jumping back into this today! A couple points related to your comments:
|
Excellent. @ndevelder I'll approve and merge after some unit tests for Sampling Utils functions are added. |
I got some of the simpler SamplingUtils tests in, but I don't have an easy way to get an independent result for the spherical cap quadrature stuff. That was effectively a direct copy from Nalu-Wind implemented by Robert K. I suppose I could try to put in something random and get a result, and just use that result to make sure it doesn't change in the future? |
Thanks for doing this, Nate! That is fine with me, if it's not too much of a lift. Just to safeguard the current version. |
Great work, Nate. Thanks for cooperating with my requests and improving the standards of the code. |
Large commit for new sampling that mimics Spinner Lidar and Radar