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

Coordinate sanity check #716

Merged
merged 16 commits into from
Jul 11, 2023
Merged

Conversation

quarz12
Copy link
Contributor

@quarz12 quarz12 commented Jul 9, 2023

Resolves #131

Proposed Changes (include Screenshots if possible)

  • checks if coordinates are within this window. If not, throw exception (maybe deleting the coords attibute works too? I noticed the interactive map shows munich if theres no coords field provided)
    image

  • computes for each room the distance to its parent building and checks if it is <100m. If not, writes a warning.

  • allows for overriding of building coordinates in coords_override.json

How to test this PR

  • run compile.py

How has this been tested?

^

Checklist

  • I have updated the documentation / No need to update the documentation
  • I have run the linter

@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2023

👋 Thank you for contributing. A staging environment for this PR for this change will be available shortly

Copy link
Member

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

  • I think there are different approaches which could catch more errors than just overlaying a rectangle globally.
    This check will, for example, not find if there is one room far, far outside the bounds of a building.
    A distance-based approach with either hardcoded or inferred distance from the room-corpus is a better approach:
    if (distance_to_parent(data) > infered_maximum_distance_between_room_and_parent[...]):
    I would prefer an approach which infers the distance based on a percentile of the data * constant factor
  • with the current approach, this won't build for
    -> I would prefer these issues to be addressed in this PR.
    This can be done by
    • deleting the coordinates (override with no coordinate),
    • overriding with the parent,
    • overriding with a manually inferred/accurate coordinate or
    • making this a warning instead of a Exception

@CommanderStorm CommanderStorm force-pushed the validate_coordinates3 branch from 085990a to 5183838 Compare July 9, 2023 16:37
@CommanderStorm
Copy link
Member

(sorry, I accidentally rebased your branch too instead of merging into it)

@quarz12
Copy link
Contributor Author

quarz12 commented Jul 9, 2023

I agree that this approach is quite inaccurate. I was not sure if I could assume building locations to be correct. That opens up way better solutions.

@quarz12
Copy link
Contributor Author

quarz12 commented Jul 9, 2023

I would prefer an approach which infers the distance based on a percentile of the data * constant factor

I am not sure I understand this right. Does this mean values should be accepted, if a certain percentile of rooms is close enough nearby?

@CommanderStorm
Copy link
Member

I was not sure if I could assume building locations to be correct

All building locations can be assumed to be correct and are to some part manually sourced.
If there is a significant variation between the rooms and the building, that would still be interesting though ^^

@quarz12
Copy link
Contributor Author

quarz12 commented Jul 9, 2023

If there is a significant variation between the rooms and the building, that would still be interesting though ^^

Well, there are a lot of rooms in 0510 that are about 2km away from their building. Looking at the 0510 coords, I'd assume that that one is incorrect though. What do you think is a good maximum distance? I did some test runs, and it looks like all correct values are easily within 100m, so I'd suggest that.

@quarz12
Copy link
Contributor Author

quarz12 commented Jul 9, 2023

I think this is now at a point where I can say that I am happy with the solution.
I know you gave me merge permissions on this repo, but I think I am still too new to this project to make this decision alone.

Copy link
Member

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

I have made some style-inline comments.
There are still quite a lot of linting issues in this PR. Please run the linter (included in pre-commit, see the Readme for further details) next time before requesting a review ^^

data/processors/coords.py Outdated Show resolved Hide resolved
data/processors/coords.py Outdated Show resolved Hide resolved
data/processors/coords.py Outdated Show resolved Hide resolved
data/processors/coords.py Outdated Show resolved Hide resolved
data/processors/coords.py Outdated Show resolved Hide resolved
data/processors/coords.py Outdated Show resolved Hide resolved
data/processors/coords.py Outdated Show resolved Hide resolved
data/processors/coords.py Outdated Show resolved Hide resolved
data/coords_override.json Outdated Show resolved Hide resolved
@CommanderStorm
Copy link
Member

I know you gave me merge permissions on this repo, but I think I am still too new to this project to make this decision alone.

Honestly, this was not intentional. I have changed a setting and this should not be possible anymore.
I was trying to give you the permission to run github-actions (like the CI) by yourself.
While for this PR this will not make a difference (#712 is not yet mergable), I thought the feedback if the docker files build could be valable feedback.

@quarz12
Copy link
Contributor Author

quarz12 commented Jul 10, 2023

Please run the linter (included in pre-commit

I thought, if I have installed that, it would automatically run? Is there a way to only run it for a single file? --all-files bloats my changes log with other files that then prevent me from switching branches until I delete them again.

I know you gave me merge permissions on this repo, but I think I am still too new to this project to make this decision alone.

Honestly, this was not intentional. I have changed a setting and this should not be possible anymore.

I did think that was a bit much😅

@CommanderStorm CommanderStorm changed the title check if coordinates are within a certain window Coordinate sanity check Jul 10, 2023
data/processors/coords.py Outdated Show resolved Hide resolved
data/processors/coords.py Outdated Show resolved Hide resolved
@CommanderStorm CommanderStorm enabled auto-merge (squash) July 11, 2023 23:32
@CommanderStorm CommanderStorm disabled auto-merge July 11, 2023 23:43
@CommanderStorm CommanderStorm merged commit 0949696 into TUM-Dev:main Jul 11, 2023
@CommanderStorm
Copy link
Member

Thank you for implementing this ❤️

@quarz12 quarz12 deleted the validate_coordinates3 branch July 12, 2023 11:02
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.

[Feature] Coordinate sainity check
2 participants