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

[Requirement] Get features within a bounding box #68

Merged
merged 18 commits into from
Jul 19, 2021

Conversation

OmarMuhammedAli
Copy link
Contributor

@OmarMuhammedAli OmarMuhammedAli commented Jul 16, 2021

Req no.8 Implementation: Get filtered features within a bbox

Purpose and Proposed Changes:

  • Implement 2 driver functions to retrieve filtered points and traffic signs within an input bounding box
  • Implement a single controller to hold the business logic and to handle both driver functions
  • Implementation and modification of multiple filtering and formatting functions to serve the same purpose of the PR

Feedback required over

What kind of feedback do you require from this PR, if any? For example,

  • A quick pair of 👀 on the code
  • Discussion on the technical approach
  • Critique on design
  • Confirmation of the validity of used logic.

Feedback required when

Whenever possible 😄

Mentions

@Rubix982, @gmelodie , and @cbeddow. Would love to discuss your thoughts and feedback!

References (OPTIONAL)

Mapillary API V4

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 16, 2021
@OmarMuhammedAli OmarMuhammedAli linked an issue Jul 16, 2021 that may be closed by this pull request
@OmarMuhammedAli OmarMuhammedAli linked an issue Jul 16, 2021 that may be closed by this pull request
@Rubix982
Copy link
Contributor

For the files,

I believe this is good to be merged.

mapillary/controller/rules/verify.py Show resolved Hide resolved
mapillary/mapillary.py Outdated Show resolved Hide resolved
mapillary/mapillary.py Outdated Show resolved Hide resolved
mapillary/utils/filter.py Outdated Show resolved Hide resolved


def haversine_dist(data: dict, radius: float, coords: list, unit: str ="m") -> dict:
def existed_at(data: list, existed_at: float) -> list:
Copy link
Member

Choose a reason for hiding this comment

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

existed_at is a good idea, I did not think of it, but I like this. If we want data that is newer than the last check we did, let us say on June 15th, then we find data where existed_at(June 15th) is false, and it will return all data from June16 or after. This is useful. Essentially the user is looking for first_seen_at > June 15th, result being new data since that time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I rename this to be existed_after instead?

Copy link
Member

Choose a reason for hiding this comment

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

No this is great

@OmarMuhammedAli
Copy link
Contributor Author

Applied suggestions and fixed a bug with date_to_unix_timestamp

@cbeddow and @Rubix982, let me know if there are any further modifications you'd like to be applied, and if this is okay to be merged!

@Rubix982
Copy link
Contributor

Approval from my side. 👍

@OmarMuhammedAli
Copy link
Contributor Author

Merging to use utils on my current PR

@OmarMuhammedAli OmarMuhammedAli merged commit bdf3b59 into main Jul 19, 2021
@OmarMuhammedAli OmarMuhammedAli deleted the get-features-in-bbox branch July 19, 2021 12:40
@OmarMuhammedAli OmarMuhammedAli restored the get-features-in-bbox branch July 19, 2021 15:28
@OmarMuhammedAli OmarMuhammedAli deleted the get-features-in-bbox branch July 19, 2021 22:20
@OmarMuhammedAli OmarMuhammedAli linked an issue Jul 20, 2021 that may be closed by this pull request
@OmarMuhammedAli OmarMuhammedAli self-assigned this Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
4 participants