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

Sorr task918 add unit test for split universe #996

Merged
merged 20 commits into from
Jun 6, 2024
Merged

Sorr task918 add unit test for split universe #996

merged 20 commits into from
Jun 6, 2024

Conversation

smitpatel49
Copy link
Contributor

sonaalKant
sonaalKant requested changes May 21, 2024
View reviewed changes
Copy link
Collaborator

@sonaalKant sonaalKant left a comment

Choose a reason for hiding this comment

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

I will also add a corner case where group is 4 and part is 2.

im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
@smitpatel49 smitpatel49 requested a review from sonaalKant May 25, 2024 00:12
gpsaggese
gpsaggese reviewed May 26, 2024
View reviewed changes
Copy link
Contributor

@gpsaggese gpsaggese left a comment

Choose a reason for hiding this comment

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

Going in the right direction, but you need to read our docs and our code base more carefully.

im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
@smitpatel49 smitpatel49 requested a review from gpsaggese May 29, 2024 00:36
@smitpatel49
Copy link
Contributor Author

smitpatel49 commented May 31, 2024 •
edited
Loading

I have updated the code and would request you to review it @gpsaggese, @sonaalKant, @samarth9008.

samarth9008
samarth9008 reviewed May 31, 2024
View reviewed changes
Copy link
Collaborator

@samarth9008 samarth9008 left a comment

Choose a reason for hiding this comment

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

Some nits.

im_v2/common/data/extract/test/test_extract_utils.py Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
@smitpatel49
Copy link
Contributor Author

Thanks for the review @samarth9008, I have updated the code accordingly.

@smitpatel49 smitpatel49 requested a review from samarth9008 May 31, 2024 22:43
@samarth9008
Copy link
Collaborator

Pls resolve the conversation if the changes are made.

@DanilYachmenev DanilYachmenev added the PR_for_authors The PR needs changes label Jun 3, 2024
@DanilYachmenev
Copy link
Collaborator

pls read https://github.com/kaizen-ai/kaizenflow/blob/master/docs/coding/all.submit_code_for_review.how_to_guide.md#save-reviewer-time, particularly part about managing PR labels
pls follow it

@smitpatel49
Copy link
Contributor Author

Hello @DanilYachmenev, I have changed the code as per your review and have also added the test with universe_part = 7.

@smitpatel49 smitpatel49 added PR_for_reviewers The PR needs to be reviewed by Team Leaders and removed PR_for_authors The PR needs changes labels Jun 3, 2024
@DanilYachmenev DanilYachmenev added PR_for_authors The PR needs changes and removed PR_for_reviewers The PR needs to be reviewed by Team Leaders labels Jun 4, 2024
@smitpatel49 smitpatel49 added PR_for_reviewers The PR needs to be reviewed by Team Leaders and removed PR_for_authors The PR needs changes labels Jun 5, 2024
DanilYachmenev
DanilYachmenev reviewed Jun 5, 2024
View reviewed changes
Copy link
Collaborator

@DanilYachmenev DanilYachmenev left a comment

Choose a reason for hiding this comment

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

fix and ready to merge

im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
im_v2/common/data/extract/test/test_extract_utils.py Outdated Show resolved Hide resolved
@DanilYachmenev DanilYachmenev added PR_for_authors The PR needs changes and removed PR_for_reviewers The PR needs to be reviewed by Team Leaders labels Jun 5, 2024
@smitpatel49 smitpatel49 added PR_for_reviewers The PR needs to be reviewed by Team Leaders and removed PR_for_authors The PR needs changes labels Jun 6, 2024
DanilYachmenev
DanilYachmenev reviewed Jun 6, 2024
View reviewed changes
Copy link
Collaborator

@DanilYachmenev DanilYachmenev left a comment

Choose a reason for hiding this comment

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

lgtm

@DanilYachmenev DanilYachmenev added PR_for_integrators The PR needs to be reviewed and possibly merged and removed PR_for_reviewers The PR needs to be reviewed by Team Leaders labels Jun 6, 2024
samarth9008
samarth9008 reviewed Jun 6, 2024
View reviewed changes
Copy link
Collaborator

@samarth9008 samarth9008 left a comment

Choose a reason for hiding this comment

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

LG

@samarth9008 samarth9008 merged commit 435c4dc into master Jun 6, 2024
1 check passed
@samarth9008 samarth9008 deleted the SorrTask918_Add_unit_test_for_split_universe branch June 6, 2024 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanilYachmenev DanilYachmenev DanilYachmenev left review comments

@samarth9008 samarth9008 samarth9008 approved these changes

@sonaalKant sonaalKant Awaiting requested review from sonaalKant

@gpsaggese gpsaggese Awaiting requested review from gpsaggese

Assignees

@smitpatel49 smitpatel49

Labels
PR_for_integrators The PR needs to be reviewed and possibly merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@smitpatel49 @samarth9008 @DanilYachmenev @sonaalKant @gpsaggese