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

Split GreaterThan into Inequality and ScalarInequality #823

Merged
merged 29 commits into from
Jun 9, 2022
Merged

Split GreaterThan into Inequality and ScalarInequality #823

merged 29 commits into from
Jun 9, 2022

Conversation

fealho
Copy link
Member

@fealho fealho commented May 26, 2022 •
edited
Loading

Resolve #814.
Resolve #817.

@codecov-commenter
Copy link

codecov-commenter commented May 26, 2022 •
edited
Loading

Codecov Report

Merging #823 (dd5c6d7) into master (04cfd65) will decrease coverage by 0.24%.
The diff coverage is 99.34%.

@@            Coverage Diff             @@
##           master     #823      +/-   ##
==========================================
- Coverage   67.98%   67.74%   -0.25%     
==========================================
  Files          38       38              
  Lines        2899     2874      -25     
==========================================
- Hits         1971     1947      -24     
+ Misses        928      927       -1
Impacted Files Coverage Δ
sdv/constraints/__init__.py 100.00% <ø> (ø)
sdv/constraints/base.py 96.13% <ø> (ø)
sdv/demo.py 21.09% <ø> (ø)
sdv/constraints/tabular.py 98.75% <99.34%> (+0.16%) ⬆️

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 f2eeffb...dd5c6d7. Read the comment docs.

@fealho fealho force-pushed the issue-814-split-greaterthan branch from ebdbda4 to e7ce5a5 Compare May 26, 2022 20:38
@fealho fealho marked this pull request as ready for review June 1, 2022 10:38
@fealho fealho requested a review from a team as a code owner June 1, 2022 10:38
@fealho fealho requested review from katxiao and removed request for a team June 1, 2022 10:38
pvk-developer
pvk-developer requested changes Jun 1, 2022
View reviewed changes
Copy link
Member

@pvk-developer pvk-developer 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 go over it again before we merge it, but for now I have those few comments.

docs/api_reference/constraints/tabular.rst Outdated Show resolved Hide resolved
docs/user_guides/single_table/handling_constraints.rst Outdated Show resolved Hide resolved
sdv/constraints/tabular.py Show resolved Hide resolved
sdv/constraints/tabular.py Outdated Show resolved Hide resolved
tests/integration/test_constraints.py Outdated Show resolved Hide resolved
amontanez24
amontanez24 reviewed Jun 1, 2022
View reviewed changes
Copy link
Contributor

@amontanez24 amontanez24 left a comment

Choose a reason for hiding this comment

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

This is looking really good so far! My main request is that we guarantee all new code has 100% test coverage. The coverage report suggests that this actually will lower the total test coverage. Can we check to make sure all of the new lines of code are covered by the unit tests?
Also, in my PR I added a file for integration tests for constraints. I think it might be worth adding at least one test for each constraint to make sure it works end to end

sdv/constraints/base.py Show resolved Hide resolved
sdv/constraints/tabular.py Outdated Show resolved Hide resolved
amontanez24
amontanez24 requested changes Jun 3, 2022
View reviewed changes
Copy link
Contributor

@amontanez24 amontanez24 left a comment

Choose a reason for hiding this comment

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

I checked the coverage report and for sdv/contraints/tabular.py, the following lines need to be tested:
https://github.com/sdv-dev/SDV/blob/issue-814-split-greaterthan/sdv/constraints/tabular.py#L304

https://github.com/sdv-dev/SDV/blob/issue-814-split-greaterthan/sdv/constraints/tabular.py#L420

https://github.com/sdv-dev/SDV/blob/issue-814-split-greaterthan/sdv/constraints/tabular.py#L441

All of them seem related to raising errors. Can we add these unit tests in?

tests/unit/constraints/test_tabular.py Outdated Show resolved Hide resolved
tests/unit/constraints/test_tabular.py Outdated Show resolved Hide resolved
tests/unit/constraints/test_tabular.py Outdated Show resolved Hide resolved
tests/unit/constraints/test_tabular.py Outdated Show resolved Hide resolved
tests/unit/constraints/test_tabular.py Show resolved Hide resolved
tests/unit/constraints/test_tabular.py Outdated Show resolved Hide resolved
@fealho fealho force-pushed the issue-814-split-greaterthan branch from f1cf8a9 to 9638ad8 Compare June 6, 2022 16:59
amontanez24
amontanez24 approved these changes Jun 6, 2022
View reviewed changes
Copy link
Contributor

@amontanez24 amontanez24 left a comment

Choose a reason for hiding this comment

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

Assuming the test coverage is now 100% this looks good to go!

@fealho fealho requested a review from pvk-developer June 7, 2022 16:49
@fealho fealho force-pushed the issue-814-split-greaterthan branch from 34dbf20 to d2a7a01 Compare June 7, 2022 17:21
@fealho fealho merged commit e4da71e into master Jun 9, 2022
@fealho fealho deleted the issue-814-split-greaterthan branch June 9, 2022 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pvk-developer pvk-developer pvk-developer approved these changes

@amontanez24 amontanez24 amontanez24 approved these changes

@katxiao katxiao Awaiting requested review from katxiao katxiao was automatically assigned from sdv-dev/core-contributors

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@fealho @codecov-commenter @amontanez24 @pvk-developer