Repository navigation
feat: add synonym support to offline geocoder - #937
daniel-heppner-ibigroup merged 5 commits into
Conversation
|
I haven't tested this in pelias-stitch, but I did add tests which make me feel comfortable with it. But up to reviewers to decide if that's good enough |
miles-grant-ibigroup
left a comment
miles-grant-ibigroup
left a comment
There was a problem hiding this comment.
Have you tried other approaches?
| ); | ||
|
|
||
| return idxs.map(index => items[index]); | ||
| const haystackMap = items.flatMap<[string, number]>((item, idx) => { |
There was a problem hiding this comment.
Confused why you created this whole haystackMap instead of just 1) appending the synonyms to the label or 2) appending the synonyms to the end of the items array and then maintaining a second list of "index overrides" for the case where one of those last items was picked.
Both of these solutions feel cleaner and like they would take fewer CPU cycles/ram consumption than this.
Sorry to be such a stickler here, but Lambda gives you 100mb of ram/storage (very strict) and we're already in the 90s
There was a problem hiding this comment.
I know we discussed using the string but I decided the memory impact of adding a few extra items is really quite small. I mean, we're only talking about a handful of extra strings here.
Appending the synonyms to the end of the items array is basically the same thing I did here except they're at the end of the array instead of next to the "parent" item. Same difference
There was a problem hiding this comment.
The overhead here is going to be KB, not MB even with a lot of synonyms
There was a problem hiding this comment.
So the problem with combined strings is that you will get some really weird results if a search term matches part of one synonym/the label and part of a subsequent synonym. I am sure I could fabricate some test cases that demonstrate this but I suspect it would actually create some really weird results in reality. I think we can afford the memory of storing more strings.
There was a problem hiding this comment.
What about approach 2? With the larger list? Wouldn't that be less code
miles-grant-ibigroup
left a comment
miles-grant-ibigroup
left a comment
There was a problem hiding this comment.
AI tests are cool but some of these are sillly
| const idxs = result.filter( | ||
| r => r.label === "Portland International Airport" | ||
| ); | ||
| expect(idxs).toHaveLength(1); |
There was a problem hiding this comment.
Maybe also expect that it returns the right result?
There was a problem hiding this comment.
that is being checked... line 53-55
| it("should return same results as autocomplete for synonym queries", async () => { | ||
| const autocompleteResult = await autocomplete({ | ||
| items: baseItems, | ||
| text: "PDX" | ||
| }); | ||
| const searchResult = await search({ items: baseItems, text: "PDX" }); | ||
| expect(searchResult).toEqual(autocompleteResult); |
There was a problem hiding this comment.
I don't know if this needs testing... Do we even want search/autocomplete to have same behavior?
There was a problem hiding this comment.
that's a separate question, as that behavior already existed in here. May as well have a test for it since this is the first test suite for this file!
|
trust me the actual AI tests were much sillier. I deleted most of them and rewrote the others. I reused the baseItems array though and most of the |
|
@daniel-heppner-ibigroup what are your thoughts on ibi-group#33 |
smaller synonym algorithm
daniel-heppner-ibigroup
assigned
amy-corson-ibigroup and unassigned
miles-grant-ibigroup
|
miles taking too long on this so i'm giving it to amy (also it's a lot of miles code now so...) |
miles-grant-ibigroup
left a comment
miles-grant-ibigroup
left a comment
There was a problem hiding this comment.
Sorry about that yeah I approve but we should probably get a third reviewer to take a peek
amy-corson-ibigroup
left a comment
amy-corson-ibigroup
left a comment
There was a problem hiding this comment.
Sorry for the delay. I think this looks good! Good tests.
amy-corson-ibigroup
assigned
daniel-heppner-ibigroup and unassigned
amy-corson-ibigroup
c0b1172
into
opentripplanner:master
4 participants
No description provided.