Add `create_custom_constraint` method #848

fealho · 2022-06-17T16:41:25Z

Resolve #836.

amontanez24

Could we also add an integration test with a CustomConstraint?

amontanez24 · 2022-06-17T20:27:16Z

sdv/constraints/tabular.py


-        return table_data
+        TYPE = 'CUSTOM_CONSTRAINT'  # this lets us know how to handle it


Can we either make this a boolean flag like is_custom or create an enum class for this? The reason would be that whenever someone is doing logic around this attribute, they would have to know the string value for custom constraints and any other type. It would be easier if they could see all the possible values. Here is an example of enums: https://docs.python.org/3/library/enum.html

Personally though, I think this can just be a boolean

codecov-commenter · 2022-06-18T16:23:04Z

Codecov Report

Merging #848 (8469200) into master (c2c2c0d) will increase coverage by 2.60%.
The diff coverage is 100.00%.

❗ Current head 8469200 differs from pull request most recent head 6ed7a81. Consider uploading reports for the commit 6ed7a81 to get more accurate results

@@            Coverage Diff             @@
##           master     #848      +/-   ##
==========================================
+ Coverage   68.04%   70.65%   +2.60%     
==========================================
  Files          38       38              
  Lines        2857     3196     +339     
==========================================
+ Hits         1944     2258     +314     
- Misses        913      938      +25

Impacted Files	Coverage Δ
sdv/constraints/__init__.py	`100.00% <ø> (ø)`
sdv/constraints/errors.py	`100.00% <100.00%> (ø)`
sdv/constraints/tabular.py	`99.62% <100.00%> (-0.11%)`	⬇️
sdv/metadata/table.py	`80.53% <100.00%> (+3.74%)`	⬆️
sdv/constraints/base.py	`96.89% <0.00%> (-0.63%)`	⬇️

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 9885db2...6ed7a81. Read the comment docs.

CLAassistant · 2022-06-23T21:11:17Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ amontanez24
✅ fealho
❌ Andrew Montanez

Andrew Montanez seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

…reject sampling

fealho · 2022-06-29T02:12:34Z

sdv/constraints/base.py

@@ -245,30 +245,6 @@ def filter_valid(self, table_data):

        return table_data[valid]

-    @classmethod
-    def from_dict(cls, constraint_dict):


This is not used anywhere else right now (and it causes problems for the changes introduced by this PR), which is why it's been deleted.

sdv/constraints/errors.py

amontanez24 · 2022-06-29T02:26:39Z

sdv/constraints/tabular.py


-            return np.logical_and.reduce(valid)
+        def _transform(self, data):


I thought we were going to override the transform method and do something to catch errors so that it is possible to fall back on reject sampling. Currently, the logic only falls back if the constraint raises a MissingConstraintColumnError, but that can't happen with a custom constraint. We either need to catch most/all errors and surface it as a MissingConstraintColumnError, or provide some special logic to handle that

commit 04d1549 Author: Felipe Alex Hofmann <fealho@gmail.com> Date: Tue Jun 28 19:49:25 2022 -0700 Move `data.copy` to base class of constraints (#849) * changing logic for handling constraints * fixing a lot of tests * removing handling_strategy attribute * removing handling_strategy from docs and code * fixing tests, docs and tutorials * adding unit tests * only calling reverse_transform if custom constraint * ignoring sre_parse during isort * pr comments * raising all errors except missing column erros * adding integration test and tracking if reverse transform should use reject sampling * refactoring how constraint transforms happen * adding unit tests * Rebase + move copy to base * Fix test cases * fix rebase * Test resulting data is a copy for transform/reverse_transform Co-authored-by: Andrew Montanez <amontanez2424@gmail.com> Co-authored-by: Andrew Montanez <andrewmontanez@Andrews-MBP.hsd1.ma.comcast.net> Co-authored-by: Andrew Montanez <andrew@datacebo.com> Co-authored-by: Andrew Montanez <andrew@sdv.dev>

amontanez24

This is looking good! 2 comments

We may want to rebase off of this branch after it gets merged to make sure nothing weird happens with the loading of the constraints: Update handling constraints doc #856
We should add a custom constraint to the integration/test_constraints.py file

…constraint

amontanez24

One small comment but besides that

tests/integration/test_constraints.py

pvk-developer

LGTM!

fealho force-pushed the issue-836-custom-constraint branch from 1d6cef1 to 05cd96e Compare June 17, 2022 16:45

amontanez24 requested changes Jun 17, 2022

View reviewed changes

amontanez24 and others added 8 commits June 22, 2022 11:15

changing logic for handling constraints

7e36919

fixing a lot of tests

e2af95a

removing handling_strategy attribute

4718be5

removing handling_strategy from docs and code

94445ca

fixing tests, docs and tutorials

a6163eb

adding unit tests

9744cc3

only calling reverse_transform if custom constraint

82f8380

ignoring sre_parse during isort

6539c67

fealho force-pushed the issue-836-custom-constraint branch from 65741a0 to 86f01bc Compare June 22, 2022 16:30

Andrew Montanez and others added 2 commits June 22, 2022 14:12

pr comments

78bd7cd

raising all errors except missing column erros

90b6003

fealho force-pushed the issue-836-custom-constraint branch from cd28d99 to fdff66a Compare June 23, 2022 21:11

fealho changed the base branch from master to issue-833-remove-handling-strategy June 23, 2022 21:11

fealho requested a review from amontanez24 June 23, 2022 21:15

amontanez24 and others added 12 commits June 23, 2022 17:23

adding integration test and tracking if reverse transform should use …

1ff251b

…reject sampling

refactoring how constraint transforms happen

7f3f74b

adding unit tests

e163b7d

Creates the custom constraint method

7ac6c5e

Add tests cases

82e4f1c

Fix lint

65cba76

Finish unit tests

8fc1577

Add integration test + fix docstrings

35e6113

Fix lint

a35d7af

Fix rebase issues

73c43eb

Move lambda functions inside methods

3029f2b

Working version using globals

d3f9ca5

fealho force-pushed the issue-836-custom-constraint branch from fdff66a to 1cdda5c Compare June 28, 2022 18:55

Base automatically changed from issue-833-remove-handling-strategy to master June 28, 2022 19:02

Merge branch 'master' into issue-836-custom-constraint

d4a8398

fealho commented Jun 29, 2022

View reviewed changes

amontanez24 reviewed Jun 29, 2022

View reviewed changes

fealho and others added 11 commits June 28, 2022 19:50

Remove double __init__

314bc75

Merge branch 'master' into issue-836-custom-constraint

162431c

Updating the handling constraint documentation

9ad91cc

fixing bugs in docs

c1fd7ef

loading constraints from dict if necessary

b705c8b

adding collapse for faq and fixing bugs

2c2df76

Change error handling for transform

b2e6337

fixing grammar error

273e80b

Add from_dict back

4caedbc

Fix lint

8469200

fealho marked this pull request as ready for review June 30, 2022 17:23

fealho requested a review from a team as a code owner June 30, 2022 17:23

fealho requested review from amontanez24 and pvk-developer June 30, 2022 17:24

amontanez24 reviewed Jun 30, 2022

View reviewed changes

fealho added 2 commits June 30, 2022 11:18

Add test case

4dbaa94

Merge branch 'update-handling-constraints-doc' into issue-836-custom-…

ba8632d

…constraint

fealho requested a review from amontanez24 June 30, 2022 18:25

amontanez24 approved these changes Jun 30, 2022

View reviewed changes

tests/integration/test_constraints.py Outdated Show resolved Hide resolved

fealho added 2 commits June 30, 2022 12:37

Merge branch 'master' into issue-836-custom-constraint

83c6b3a

Updtae integration test

6ed7a81

pvk-developer approved these changes Jun 30, 2022

View reviewed changes

fealho merged commit 0f88065 into master Jul 1, 2022

fealho deleted the issue-836-custom-constraint branch July 1, 2022 06:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `create_custom_constraint` method #848

Add `create_custom_constraint` method #848

fealho commented Jun 17, 2022

amontanez24 left a comment

amontanez24 Jun 17, 2022

codecov-commenter commented Jun 18, 2022 •

edited

Loading

CLAassistant commented Jun 23, 2022 •

edited

Loading

fealho Jun 29, 2022 •

edited

Loading

amontanez24 Jun 29, 2022

amontanez24 left a comment •

edited

Loading

amontanez24 left a comment

pvk-developer left a comment


		return table_data
		TYPE = 'CUSTOM_CONSTRAINT' # this lets us know how to handle it


		return np.logical_and.reduce(valid)
		def _transform(self, data):

Add create_custom_constraint method #848

Add create_custom_constraint method #848

Conversation

fealho commented Jun 17, 2022

amontanez24 left a comment

Choose a reason for hiding this comment

amontanez24 Jun 17, 2022

Choose a reason for hiding this comment

codecov-commenter commented Jun 18, 2022 • edited Loading

Codecov Report

CLAassistant commented Jun 23, 2022 • edited Loading

fealho Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

amontanez24 Jun 29, 2022

Choose a reason for hiding this comment

amontanez24 left a comment • edited Loading

Choose a reason for hiding this comment

amontanez24 left a comment

Choose a reason for hiding this comment

pvk-developer left a comment

Choose a reason for hiding this comment

Add `create_custom_constraint` method #848

Add `create_custom_constraint` method #848

codecov-commenter commented Jun 18, 2022 •

edited

Loading

CLAassistant commented Jun 23, 2022 •

edited

Loading

fealho Jun 29, 2022 •

edited

Loading

amontanez24 left a comment •

edited

Loading