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

(1) Fix place quantified name retrieval issues (2) Use s2 cell-based search for all hazard search scenarios #274

Merged
merged 8 commits into from
May 25, 2022

Conversation

zilongliu-geo
Copy link
Member

(1) Fix place quantified name retrieval issues (2) Use s2 cell-based search for all hazard search scenarios

@ThomasThelen
Copy link
Member

Looks great! In this area, since placeEntities and placesConnectedToS21 are both arrays you can just add them together rather than iterating over placeEntities and adding it element by element.

This line looks off to me since formattedResults[counterRow]['place_name'] should be set above in those if statements.

Something seems off with the quantified name merge because some of the code that I wrote for that is getting reverted in this PR. For example, this was changed to use .forEach and takes advantage of the built in counter so that we don't have to manage the index ourselves. I would revert the branch to the pre-quantified name changes, merge it with stko-kwg, commit the merge changes, then make your changes, then commit those changes. I shouldn't be seeing any changes to the quantifiedName code here since it's already in stko-kwg (and I don't want to re-test the feature)

Can we also delete the code that was commented out? I think it's safe to get rid of it if we're not using it.

@zilongliu-geo
Copy link
Member Author

@ThomasThelen The commented codes are now removed. The issue that I solved with placeQuantName is to use group_concat for the retrieval. That is why in the line you highlighted I have to split the processed place_name into an array.

@ThomasThelen
Copy link
Member

Thanks for removing it! From what I understand, quantified name isn't broken in stko-kwg, which is why I don't like seeing any changes to that area of the code here. The group concat for placeQuantName in your branch is the same as the one in stko-kwg except in your branch it moved position in the query projection (do a ctl+f search for (group_concat(distinct ?placeQuantName; separator = "||") and you'll se the line is the same in both branches) . I also see some .split('||')[0] removed - but I'm not seeing why this needs to happen. Is there something broken with quantified name?

@zilongliu-geo
Copy link
Member Author

@ThomasThelen Oh no! Definitely something is wrong here. I merged stko-kwg to my branch before, but it didn't contain your changes. So I thought I should fix it, and now it turns out that something lost during the merge.

@ThomasThelen
Copy link
Member

My guess is the auto merge chose the changes on your branch vs the ones on stko-kwg. I would save your work in this branch, revert your branch back to before that merge, then do the merge and add your work in

@zilongliu-geo
Copy link
Member Author

@ThomasThelen Thanks!

@ThomasThelen
Copy link
Member

I finished testing this and it looks great! Glad we were able to get rid of some of the complexity in the query generation code too

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.

Unable to search for hurricanes in Louisiana
2 participants