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.

Sign up for GitHub

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

Jump to bottom

Location extraction #371

Merged
merged 7 commits into from
Oct 17, 2023
Merged

Location extraction #371

merged 7 commits into from
Oct 17, 2023

Conversation

LeonardPuettmannKern
Copy link
Contributor

@LeonardPuettmannKern LeonardPuettmannKern commented Oct 9, 2023 •
edited
Loading

refinery

  • Tested by creator on refinery
  • Tested by reviewer on refinery
  • Ensured that output of brick conforms with refinery structure (to be checked by reviewer)

API

  • Tested by creator on localhost:8000/docs
  • Tested by reviewer on localhost:8000/docs

common code

  • Common code tested in notebook/ script by creator
  • Common code tested in notebook/ script by reviewer
  • Common code contains docstrings and type hints

additional points:

  • Docstring and README is existing
  • Import statements (in __init__.py)
  • (If necessary) Added dependency to requirements.txt
  • (If necessary) Added dependency to issue for refinery env here
  • Published brick to Strapi CMS (locally)

@JWittmeyer This brick is not able to extract whole addresses, but might be a simple and easy replacement until the address extraction is improved.

Implements #369

Answer jwittmeyer:
yes i think this works great for a general overview

FelixKirschKern
FelixKirschKern approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@FelixKirschKern FelixKirschKern left a comment

Choose a reason for hiding this comment

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

can be merged, after changing the typing issue

extractors/personal_identifiers/location_extraction/code_snippet_common.md Outdated Show resolved Hide resolved
@LeonardPuettmannKern LeonardPuettmannKern merged commit 87b784f into main Oct 17, 2023
@LeonardPuettmannKern LeonardPuettmannKern deleted the location-extraction branch October 17, 2023 15:12
This was referenced Oct 18, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JWittmeyer JWittmeyer JWittmeyer left review comments

@FelixKirschKern FelixKirschKern FelixKirschKern approved these changes

@jhoetter jhoetter Awaiting requested review from jhoetter

Assignees
No one assigned
Labels
cognition
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LeonardPuettmannKern @JWittmeyer @FelixKirschKern