Skip to content

Add a new optional argument tid in DataStore.add_entries #779

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

Merged
merged 27 commits into from
May 24, 2022
Merged

Add a new optional argument tid in DataStore.add_entries #779

merged 27 commits into from
May 24, 2022

Conversation

hepengfe
Copy link
Collaborator

@hepengfe hepengfe commented May 6, 2022

This PR fixes #775 and #781

Description of changes

  • Add a new optional argument tid in DataStore.add_entries
  • implemented add_link_raw and add_group_raw
  • implemented _add_entry_raw to help modularizing add_annotation_raw, add_link_raw and add_group_raw

Possible influences of this PR.

Future functions that add new types of entries in raw format can use _add_entry_raw

Test Conducted

  • test add_link_raw and add_group_raw

@codecov
Copy link

codecov bot commented May 6, 2022

Codecov Report

Merging #779 (6229729) into master (1fbb483) will increase coverage by 0.06%.
The diff coverage is 94.54%.

@@            Coverage Diff             @@
##           master     #779      +/-   ##
==========================================
+ Coverage   81.02%   81.09%   +0.06%     
==========================================
  Files         249      249              
  Lines       18716    18748      +32     
==========================================
+ Hits        15164    15203      +39     
+ Misses       3552     3545       -7     
Impacted Files Coverage Δ
forte/data/data_store.py 90.97% <91.42%> (+3.18%) ⬆️
tests/forte/data/data_store_test.py 93.82% <100.00%> (+0.43%) ⬆️

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 1fbb483...6229729. Read the comment docs.

@hepengfe hepengfe requested a review from mylibrar May 6, 2022 19:42
@hepengfe hepengfe marked this pull request as ready for review May 7, 2022 04:40
@hepengfe hepengfe self-assigned this May 9, 2022
@hepengfe hepengfe marked this pull request as draft May 9, 2022 20:48
@hepengfe hepengfe marked this pull request as ready for review May 9, 2022 21:46
@mylibrar mylibrar requested a review from hunterhector May 9, 2022 21:56
@hunterhector hunterhector requested a review from wanglec May 9, 2022 23:05
entry: raw entry data in the list format.

Raises:
ValueError: raised when the entry type's sorting function is not
Copy link
Member

Choose a reason for hiding this comment

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

No ValueError is raised in this function, I only see KeyError

@mylibrar mylibrar added this to the 0.3 alpha milestone May 20, 2022
@mylibrar mylibrar requested a review from hunterhector May 23, 2022 20:15
@mylibrar mylibrar linked an issue May 23, 2022 that may be closed by this pull request
num_link = len(
self.data_store._DataStore__elements["forte.data.ontology.top.Link"]
)
self.assertEqual(num_link, 2)
Copy link
Member

Choose a reason for hiding this comment

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

should probably test whether the tid is assigned correctly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added the test.

@@ -384,12 +396,13 @@ def _new_group(self, type_name: str, member_type: str) -> List:
Args:
type_name: The fully qualified type name of the new entry.
member_type: Fully qualified name of its members.
tid: ``tid`` of the ``Group`` entry.
Copy link
Member

Choose a reason for hiding this comment

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

This, and other occurrences of tid, should expalin that tid can be optional and will be auto-assigned.

@mylibrar mylibrar merged commit 961c5df into asyml:master May 24, 2022
@hepengfe hepengfe deleted the add_entries_w_id branch May 24, 2022 16:33
This was referenced May 24, 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.

Modularize `add_entry_raw(entry_type, ...) Add a new optional argument tid in DataStore.add_entries
3 participants