-
Notifications
You must be signed in to change notification settings - Fork 27
Adds sticky labels to Project Sidewalk explore #3949
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
base: develop
Are you sure you want to change the base?
Conversation
|
Amazing work @jsomeara. Kudos! Are we sure that this technique will always offer more lat,lng precision than previous techniques? Your "very important" note suggests this is the case but I wonder if we have any data backing that claim... |
I don't have any data backing that up right now. Should I do some testing to get that data? I imagine it would involve creating ground truth locations using aerial imagery and then seeing which method |
|
Yes, I think just doing a bit of testing would be warranted given the significant of this change. Could you somehow use the ICCV ground truth dataset as a test set? Probably not since those panos were labeled outside of PS... |
Yeah exactly. I wouldn't be able to use them. |
|
Hrmph, well, I still think it's worth doing an investigation before running a script that literally changes every lat,lng we have in PS. That feels like something that deserves some thorough analysis. Thoughts @misaugstad? |
|
I definitely agree that we need to do an investigation. Sorry if my previous message made it seem otherwise. |
|
Definitely not! Just wanted to make sure we were on the same page. This is amazing work @jsomeara, as usual. THANK YOU! |
misaugstad
left a comment
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.
I added a few quick comments from looking at the code!
|
@misaugstad Thank you for the comments :D |
Investigation on GSV Location Extraction Method vs. Regression ApproximationMethod: I randomly sampled 100 PS curb ramp labels and extracted their location using the regression method (old method) and the gsv method (new method). These extracted locations were overlayed onto aerial imagery. For each PS label, I manually classified which one visually appeared to be closer to the actual curb ramp (per the aerial imagery). If it was hard to tell where the actual curb ramp was (e.g., due to trees occlusion, shadows, low res, low contrast), I classified as "can't tell who wins." Results on the investigation:
Wow! That's definitely a surprising result for me. I thought for sure the new GSV-method would win. What I observed was that often, the PS users' labels would be misplaced ever so slightly that it would completely throw off the GSV-method, while the old approximation method is much more tolerant to users' impreciseness. I think the reason I thought the GSV-method would be so much better is because during testing, I was only really testing with points that I knew to be accurate. I think this means we should revert back to the old method for converting marker -> lat lon, but we can still use the new method to convert lat lon -> marker, enabling sticky labels. But maybe it's better to use the regression method for lat lon -> marker as well? Not really sure. https://github.com/ProjectSidewalk/gsv-location-extraction-analysis |
|
Wow, thanks for this analysis. I wonder if we should extend it to ~200 labels (across label types) and check again.
Agree
I think this requires more investigation, no? |
We are limited in the label types we can use because not all are easily pinpointable in the aerial imagery. We could probably do curb ramp, missing curb ramp, and crosswalk. Is there any reason to expect different results for different label types? |
|
Well, only because you said placement was a factor--and different label types tend to have slightly different labeling placements. But I trust your instincts on this as well as the ROI on further investigations. |
|
We now save both GSV and regression-calculated coords in SQL DB. Regression-calculated coords are used primarily, but for the sticky labels, the GSV-calculated location coords are used. For a fallback, we can still use regression-calculated coords for the sticky labels. Ready for review. |
…Webpage into 3353-label-does-not-stick
|
BTW, we shouldn't merge this PR until we have a script that can add GSV-extracted location coords to the existing DB. |
|
Noted! |
misaugstad
left a comment
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.
Looking great! Main thought is just that gsvLat and gsvLng probably aren't the best names... It sounds like we're talking about the lat/lng of the camera that took the pano image. Something like latUsingGsv/lngUsingGsv is kinda clunky but is at least more clear. Very open to naming suggestions!
The other idea is that we really shouldn't be storing latitude and longitude as separate columns in the database. It's what we've always done, but we should really be storing them as "points", making use of the fact that we're using postgrs and postgis. I made a ticket for changing all the data that we already have (#3909) that we'll deal with in the future, but we should at least be doing it the "right" way for all new columns that we add!
The typical naming convention would be to call the point geometry column geom. Since what we're doing here is adding a secondary geometry column (the lat and lng are our "primary" geometry columns), we could call it geomUsingGsv for example (if we go with my first proposed name change).
|
Thank you for the review!! |
|
Ready for review! |
…Webpage into 3353-label-does-not-stick
|
Ah, what do you mean? Do you mean that the labels still show up even if like, you turn a corner, and there is now a tree in the way--but the "sticky label" is still showing and now looks like it's labeled on a tree? |
|
Yes! Like in that example, I was labeling adding No Sidewalk labels to the street that was on the other side of that building. I suppose that using No Sidewalk labels wasn't super obvious here... But you can kind of see from the minimap where I was trying to label |
misaugstad
left a comment
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.
Looks good! I mostly just added a few more comments and merged in develop. This is awesome work!!
|
I figured that's what you were trying to say but the screenshot was too ambiguous to know for sure. I think this is actually a big problem and could be sufficiently large to warrant not including this in production. Why? Because it's confusing, especially to new users... and they could end up deleting properly placed labels simply because they look completely wrong once you turn a corner, etc. So, perhaps two solutions:
|
|
I really like those first two suggestions! The third sounds quite tricky to get right, as you know! |

Resolves #3353
This PR adds sticky labels to the Project Sidewalk explore page using the newly discovered
fromContainerPixelToLatLngandfromLatLngToContainerPixelfunctions.Some important considerations were made:
Very important:
This updates all future lat lon locations of labels to be calculated using the new GSV method. However, labels created before this change will be very inaccurate and unsuitable for sticky labels. We will need to run a separate script to update these old labels when we implement this change into production.
Video
8mb.video-6n0-ZzdcCyf4.mp4
Testing instructions
Things to check before submitting the PR