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

ADD: Time tolerance to GateMapper #1093

Merged
merged 3 commits into from
Mar 29, 2022
Merged

ADD: Time tolerance to GateMapper #1093

merged 3 commits into from
Mar 29, 2022

Conversation

rcjackson
Copy link
Collaborator

Added a time tolerance to the GateMapper function. Now you can exclude points that are out of synchronization when doing gate to gate comparisons.

@rcjackson rcjackson requested review from zssherman and mgrover1 March 29, 2022 16:49
Copy link
Collaborator

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

Small name change

@@ -50,32 +50,43 @@ class GateMapper(object):
The gatefilter to apply to the source radar data before mapping
tol: float
The difference in meters between the source and destination gate allowed for an adequate match.
time_tol: float
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you rename both tol/time_tol here? Changing it to distance_tolerance, and time_tolerance so it is a bit more explicit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've put in a new commit changing the names of these two variables to be more explicit throughout.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome - the example will need an update too... which is why the doc build is failing

@mgrover1
Copy link
Collaborator

@rcjackson just a couple of suggestions in terms of naming... I think this will make it clearer for people using this API

@mgrover1
Copy link
Collaborator

@rcjackson I added the example doc update here since the api changed slightly

Copy link
Collaborator

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

Alrighty! This looks good to go 👍

@mgrover1 mgrover1 merged commit 1f88c2b into ARM-DOE:main Mar 29, 2022
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.

3 participants