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

Function to remove data pack from a multi pack #555

Merged
merged 31 commits into from
Feb 23, 2022
Merged

Conversation

J007X
Copy link
Collaborator

@J007X J007X commented Nov 4, 2021

This PR fixes #544.

Description of changes

remove_pack(index_of_pack: int, clean_invalid_entries: bool = False) is added with clean_invalid_entries option for removing Cross-Pack links/groups associated with the data pack to be removed. If this flag is false and the pack to be removed have cross-pack links/groups, a ValueError will be raised indicate user the existence of cross pack links/groups with this pack and the need for setting the flag to be true to remove the associated links/groups.

Possible influences of this PR.

Possible side-effects: if directly removed packs from related arrays the index for the elements after that will be changed which will affect those buffered index such as in getChild/getParent. So current solution is to set the element in corresponding arrays as None instead to keep the index of other elements unchanged. A purge function is also added however to compress the array and remove the None in it however this will cause the index of elements after those removed ones being changed.

Test Conducted

Unit test performed for remove single pack, remove pack with cross pack links and groups. Also unit tested purge function.

@codecov
Copy link

codecov bot commented Nov 4, 2021

Codecov Report

Merging #555 (812c5da) into master (63577ce) will increase coverage by 0.08%.
The diff coverage is 89.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #555      +/-   ##
==========================================
+ Coverage   80.82%   80.90%   +0.08%     
==========================================
  Files         240      240              
  Lines       17194    17362     +168     
==========================================
+ Hits        13897    14047     +150     
- Misses       3297     3315      +18     
Impacted Files Coverage Δ
forte/data/multi_pack.py 78.01% <75.51%> (-0.53%) ⬇️
tests/forte/data/multi_pack_test.py 96.68% <94.95%> (-2.24%) ⬇️

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 63577ce...812c5da. 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.

Since we will change some implementation in pack id, I didn't fully review this. But I added some comments on some other parts.

forte/data/multi_pack.py Outdated Show resolved Hide resolved
forte/data/multi_pack.py Outdated Show resolved Hide resolved
forte/data/multi_pack.py Outdated Show resolved Hide resolved
forte/data/multi_pack.py Outdated Show resolved Hide resolved
forte/data/multi_pack.py Outdated Show resolved Hide resolved
forte/data/multi_pack.py Outdated Show resolved Hide resolved
forte/data/multi_pack.py Outdated Show resolved Hide resolved
forte/data/multi_pack.py Outdated Show resolved Hide resolved
tests/forte/data/multi_pack_test.py Outdated Show resolved Hide resolved
tests/forte/data/multi_pack_test.py Outdated Show resolved Hide resolved
…"None" type cause a secondary exception issue 3) in multi_pack_test.py member variable to local variable changes per pull request comments from Hector
@J007X
Copy link
Collaborator Author

J007X commented Nov 28, 2021

Please have a review of the new code that fixed the comments issue as suggested. Thanks.

@hunterhector hunterhector changed the title Bug544fix Function to remove data pack from a multi pack Dec 7, 2021
forte/data/multi_pack.py Show resolved Hide resolved
forte/data/multi_pack.py Outdated Show resolved Hide resolved
forte/data/multi_pack.py Show resolved Hide resolved
forte/data/multi_pack.py Outdated Show resolved Hide resolved
forte/data/multi_pack.py Show resolved Hide resolved
forte/data/multi_pack.py Outdated Show resolved Hide resolved
forte/data/multi_pack.py Show resolved Hide resolved
@hunterhector
Copy link
Member

any updates to the reviews

…ic would be set to None and which set to empty
@hunterhector
Copy link
Member

Should update branch with master and see if we can pass CI

@J007X
Copy link
Collaborator Author

J007X commented Feb 3, 2022 via email

forte/data/multi_pack.py Outdated Show resolved Hide resolved
forte/data/multi_pack.py Show resolved Hide resolved
@hunterhector hunterhector merged commit 4726033 into master Feb 23, 2022
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.

Add a datapack remover function for multipack
2 participants