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

Add support for unit type numbered #87

Merged
merged 5 commits into from
May 4, 2020
Merged

Add support for unit type numbered #87

merged 5 commits into from
May 4, 2020

Conversation

Joxit
Copy link
Member

@Joxit Joxit commented Apr 13, 2020

Hi there !

I added some support for unit 🎉

Classifications

  • UnitTypeClassification: public classifications for types like unit, apt...
  • UnitClassification: public classification for unit numbers

Classifiers

  • UnitTypeClassifier: classify a UnityType thanks to libpostal unit_types_numbered.txt (I removed stop and shop)
  • UnitClassifier: classify a Unit, this must be after a unit type
  • UnitTypeUnitClassifier: complex classifier for units like U12. This will parse a span and will add two new spans U and 12 in the section with the proper classification.

This fixes #71 and need more work for #69

@Joxit Joxit mentioned this pull request Apr 17, 2020
@missinglink
Copy link
Member

This looks very promising 👀

@Joxit
Copy link
Member Author

Joxit commented Apr 20, 2020

I think I will merge this this week and do another PR for #69 if everything is OK 😄

@missinglink
Copy link
Member

Yeah very nice, could you please update the mask functionality?

We use that in pelias/api

@Joxit
Copy link
Member Author

Joxit commented Apr 20, 2020

Are you OK with this one ? U for both unit and unit_type ?

@Joxit
Copy link
Member Author

Joxit commented Apr 20, 2020

Different result order between node 10 and node 12 🤦‍♂️
Ok, I need to fix this too...

@Joxit
Copy link
Member Author

Joxit commented Apr 20, 2020

Should be ok now 😄

@missinglink
Copy link
Member

Nice, yeah that all looks great ⭐

@missinglink
Copy link
Member

Additional test case Lot 2, Burrows Avenue, EDMONDSON PARK, NSW, Australia

@Joxit
Copy link
Member Author

Joxit commented Apr 30, 2020

This new test is passing 👍

But if I merge this now, it will fail new test from #88 because there are 4 children Beethovenova 641/9 641 and 9

 address CZEs: Beethovenova 641/9, Brno
    1m✖ should be equivalent
    -----------------------
      operator: deepEqual
      expected: |-
        [ { street: 'Beethovenova' }, { housenumber: '641/9' }, { locality: 'Brno' } ]
      actual: |-
        undefined
      at: Test.<anonymous> (/home/travis/build/pelias/parser/test/common.js:22:9)
      stack: |-
  Error: should be equivalent
  at Test.assert [as _assert] (/home/travis/build/pelias/parser/node_modules/tape/lib/test.js:228:54)
  at Test.bound [as _assert] (/home/travis/build/pelias/parser/node_modules/tape/lib/test.js:80:32)
  at Test.tapeDeepEqual (/home/travis/build/pelias/parser/node_modules/tape/lib/test.js:426:10)
  at Test.bound [as deepEquals] (/home/travis/build/pelias/parser/node_modules/tape/lib/test.js:80:32)
  at Test.<anonymous> (/home/travis/build/pelias/parser/test/common.js:22:9)
  at Test.bound [as _cb] (/home/travis/build/pelias/parser/node_modules/tape/lib/test.js:80:32)
  at Test.run (/home/travis/build/pelias/parser/node_modules/tape/lib/test.js:96:10)
  at Test.bound [as run] (/home/travis/build/pelias/parser/node_modules/tape/lib/test.js:80:32)
  at Immediate.next [as _onImmediate] (/home/travis/build/pelias/parser/node_modules/tape/lib/results.js:81:19)
  at processImmediate (internal/timers.js:456:21)

@missinglink
Copy link
Member

missinglink commented Apr 30, 2020

We've never had 'words' with overlapping character ranges before:

641/9  13:18
641    13:16
    9  17:18

Is it possible to remove the 641/9 'word' so that's no longer the case?

@Joxit Joxit force-pushed the joxit/feat/unit branch from e7ce1fb to 0e592a1 Compare May 1, 2020 19:33
@Joxit
Copy link
Member Author

Joxit commented May 1, 2020

This was introduced in #56, I named it Alternative spans. I added in this PR /

Now we should navigate through the graph.

I fixed the test 😄

@missinglink
Copy link
Member

Cool 👍 we can address the graph relationships in #47

@Joxit Joxit merged commit dd24aaf into master May 4, 2020
@Joxit Joxit deleted the joxit/feat/unit branch May 4, 2020 16:37
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.

Street number prefixes
2 participants