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

Parse user specified ontology file #692

Merged
merged 36 commits into from
Jun 27, 2022
Merged

Parse user specified ontology file #692

merged 36 commits into from
Jun 27, 2022

Conversation

qinzzz
Copy link
Collaborator

@qinzzz qinzzz commented Mar 23, 2022

This PR handles the parameter onto_file_path in DataStore's init method.

Description of changes

If onto_file_path is not None, build tyoe_attribute dictionary based on the provided ontology file.

Possible influences of this PR.

Describe what are the possible side-effects of the code change.

Test Conducted

test_check_onto_file in data store test.

@codecov
Copy link

codecov bot commented Mar 23, 2022

Codecov Report

Merging #692 (0a75ae2) into master (5e966b9) will increase coverage by 0.05%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##           master     #692      +/-   ##
==========================================
+ Coverage   80.35%   80.40%   +0.05%     
==========================================
  Files         253      253              
  Lines       19315    19358      +43     
==========================================
+ Hits        15520    15565      +45     
+ Misses       3795     3793       -2     
Impacted Files Coverage Δ
forte/data/data_store.py 82.55% <95.65%> (+1.62%) ⬆️
forte/common/constants.py 100.00% <100.00%> (ø)
forte/data/ontology/ontology_code_const.py 100.00% <100.00%> (ø)
tests/forte/data/data_store_test.py 95.46% <100.00%> (+0.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e966b9...0a75ae2. Read the comment docs.

Copy link
Member

@hunterhector hunterhector left a comment

Choose a reason for hiding this comment

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

try to reuse the ontology code generator

forte/data/data_store.py Outdated Show resolved Hide resolved
forte/data/data_store.py Outdated Show resolved Hide resolved
forte/data/data_store.py Outdated Show resolved Hide resolved
Copy link
Member

@hunterhector hunterhector left a comment

Choose a reason for hiding this comment

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

In additional to the comments, there are a few more general steps missing:

  1. We always need to associate a PR with an issue for tracking purposes.
  2. Need to resolve conflicts.

forte/data/data_store.py Outdated Show resolved Hide resolved
forte/data/data_store.py Outdated Show resolved Hide resolved
forte/data/data_store.py Outdated Show resolved Hide resolved
forte/data/data_store.py Outdated Show resolved Hide resolved
forte/data/data_store.py Outdated Show resolved Hide resolved
forte/data/data_store.py Outdated Show resolved Hide resolved
forte/data/data_store.py Outdated Show resolved Hide resolved
@qinzzz qinzzz force-pushed the onto branch 2 times, most recently from d5806e0 to 35596ab Compare March 30, 2022 18:59
@hunterhector
Copy link
Member

let's pass the CI?

@hunterhector
Copy link
Member

@mylibrar

forte/data/data_store.py Outdated Show resolved Hide resolved
forte/data/data_store.py Outdated Show resolved Hide resolved
Copy link
Member

@hunterhector hunterhector left a comment

Choose a reason for hiding this comment

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

For this PR let's focus on making the test case covering what we want.

tests/forte/data/data_store_test.py Outdated Show resolved Hide resolved
@mylibrar mylibrar merged commit 52857c2 into asyml:master Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants