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

Added a concept dict creator function #58

Merged
merged 7 commits into from
Feb 9, 2024
Merged

Added a concept dict creator function #58

merged 7 commits into from
Feb 9, 2024

Conversation

msnackey
Copy link
Collaborator

@msnackey msnackey commented Feb 2, 2024

  • Adds a function for creating a concept dictionary based on a .csv-file
  • The .csv-file needs to have valid column names based on available attributes for the clinlp Term class

Closes #56.

@msnackey msnackey requested a review from vmenger February 2, 2024 16:24
Copy link
Collaborator

@vmenger vmenger left a comment

Choose a reason for hiding this comment

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

Nog een paar algemene comments:

  • De linters/formatting pipeline test geeft een error, je kunt even proberen uit te zoeken waar dat door komt
  • Er missen type hints, dat zit in de hele repository wel dus is het hier ook wel wenselijk (weet even niet zeker of een pipeline dit ook enforcet)
  • Er missen nog tests
  • Er mist nog een stukje in de documentatie met uitleg en voorbeeld

clinlp/entity.py Outdated Show resolved Hide resolved
clinlp/entity.py Outdated Show resolved Hide resolved
clinlp/entity.py Outdated Show resolved Hide resolved
clinlp/entity.py Outdated Show resolved Hide resolved
clinlp/entity.py Outdated Show resolved Hide resolved
clinlp/entity.py Outdated Show resolved Hide resolved
clinlp/entity.py Outdated Show resolved Hide resolved
clinlp/entity.py Outdated Show resolved Hide resolved
clinlp/entity.py Outdated Show resolved Hide resolved
clinlp/entity.py Outdated Show resolved Hide resolved
@msnackey msnackey requested a review from vmenger February 6, 2024 15:04
Copy link
Collaborator

@vmenger vmenger left a comment

Choose a reason for hiding this comment

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

Ziet er goed uit. Deze twee functies mogen trouwens tussen Term en EntityMatcher in komen te staan. Geeft mooie opbouw van dependencies!

clinlp/entity.py Outdated Show resolved Hide resolved
clinlp/entity.py Outdated Show resolved Hide resolved
clinlp/entity.py Outdated Show resolved Hide resolved
clinlp/entity.py Outdated Show resolved Hide resolved
tests/unit/test_entity.py Outdated Show resolved Hide resolved
tests/unit/test_entity.py Outdated Show resolved Hide resolved
@msnackey msnackey merged commit 94758d9 into main Feb 9, 2024
3 checks passed
@msnackey msnackey deleted the cdc branch February 9, 2024 08:10
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.

Parsing entity matching concepts with settings from csv/excel
2 participants