-
Notifications
You must be signed in to change notification settings - Fork 28
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
QARTOD.Location_test: Add a range from target feature #46
base: master
Are you sure you want to change the base?
QARTOD.Location_test: Add a range from target feature #46
Conversation
Thanks @JessyBarrette! The doc-string of the
It might make sense to separate these into three different tests and the result of the |
@kwilcox You're right about the different tests, yes! Regarding how to structure the different tests. I think separating them as you suggest would make a lot of sense. If only one of the tests can be run at the time and the different tests are aggregated after, it can definitely make it easier to identify each test result, troubleshoot issues, and just make it cleaner to handle. Another point maybe, I think each of those tests should have the potential to apply either a SUSPECT and/or FAIL result and leave it to the user to decide which one is best for them to apply. |
@JessyBarrette I'm back on |
@kwilcox sounds good we're all scrambling these days. I figured you were busy |
@JessyBarrette sadly Kyle is no longer working on this and we ar trying to revisit the open PRs in ioos_qc. It seems that the only missing issue here is how to organize the tests, right? Is this something you are still wiling to work on? PS: I'm not super familiar with the QARTOD manuals. Do you mind posting the manual/page this test is from so we can document it. |
This is pretty old haha. From page 16 of the Manual for Real-Time Quality Control of In-situ Temperature and Salinity Data I realize now this optional test do not match the description of the I do see the need for such test in some particular cases (ex: Buoy drifting from an initial target mooring location). But potentially this should not live within the QARTOD module. I am open to the idea of either dropping this PR or moving this test to another module. |
I'll leave that to the new dev @iwensu0313. But I agree, we should not overload the qartod module with it and it is a valuable test to have somewhere. |
@JessyBarrette I like your new test ideas as well. I think if we want to make amendments to the QARTOD tests, we would want to propose this to the IOOS community. @ocefpaf / IOOS shared that the manuals are moving to github now (qartod_manuals), which might make it easier to collect feedback and update the manuals more frequently moving forward. I am only familiar w/ the QARTOD implementation in Maybe @lukecampbell you have some immediate thoughts? |
The present pull request is adding three optional inputs to the location_test that define a target location (target_lon [degree_east], target_lat [degree_north]) and an acceptable distance from the target (target_range [m]). target_lon and target_lat can either be a single value or an array the same size as the lon and lat inputs.
The location_test output a SUSPECT flag if the distance from the target is exceeding the provided target_range.
As an example, such location_test can be useful to test:
Although not necessarily considered to be used that way, it could also potentially be used to compare a mobile platform trajectory versus a predefined trajectory. A new tool distance_from_target is also added to the ioos_qc.utils module to compute the distance between two locations.
A series of tests were also added to the test_qartod module to test those new inputs.