Add constraint methods to BaseMultiTableSynthesizer
#1178
Conversation
| Raises when the ``Unique`` constraint is passed. | ||
| """ | ||
| if self._fitted: | ||
| warnings.warn( |
There was a problem hiding this comment.
Adding the warning here so it only shows once. If I used the SingleTable's add_constraints warning, it would show for every constraint in the list.
I'll add a test for the warning after you thumbs up this.
sdv/multi_table/base.py
Outdated
| for constraint in constraints: | ||
| if constraint['constraint_class'] == 'Unique': | ||
| raise SynthesizerInputError( | ||
| "The constraint class 'Unique' is not currently supported." |
There was a problem hiding this comment.
I updated the message a little from the issue.
There was a problem hiding this comment.
maybe add to the end of this sentence "for multi-table synthesizers" so users don't think it's not supported at all
fealho
requested review from
katxiao,
amontanez24 and
pvk-developer
and removed request for
a team and
katxiao
sdv/multi_table/base.py
Outdated
| if constraint['constraint_class'] == 'Unique': | ||
| raise SynthesizerInputError( | ||
| "The constraint class 'Unique' is not currently supported." | ||
| 'Please remove the constraint for this synthesizer.' | ||
| ) |
There was a problem hiding this comment.
Could we pull this in a function before this loop, to ensure that no other constraint was added to the data processor. Currently we have no way to remove or reset the constraints, @amontanez24 maybe we can add something for it ?
There was a problem hiding this comment.
That may be something we add later
There was a problem hiding this comment.
I agree though that if we can't add this constraint, we should either add all of the other ones, or none of the other ones. We don't want to crash half way through
tests/unit/multi_table/test_base.py
Outdated
| positive_constraint.pop('table_name') | ||
| negative_constraint.pop('table_name') | ||
| assert instance._table_synthesizers['nesreca'].get_constraints() == [positive_constraint] | ||
| assert instance._table_synthesizers['oseba'].get_constraints() == [negative_constraint] |
There was a problem hiding this comment.
Can we re-write this as a dict. The reason why is that if positive_constraint is being modified down the line and it's not stored as expected we may not capture it.
tests/unit/multi_table/test_base.py
Outdated
| constraints = [positive_constraint, negative_constraint] | ||
|
|
||
| # Run | ||
| instance.add_constraints(constraints) |
There was a problem hiding this comment.
This is part of the setup.
tests/unit/single_table/test_base.py
Outdated
| instance.add_constraints([]) | ||
|
|
||
| def test_add_constraints(self): | ||
| """Test a list of constraits can be added to the synthesizer.""" |
tests/unit/single_table/test_base.py
Outdated
| assert instance.get_constraints() == [positive_constraint, negative_constraint] | ||
|
|
||
| def test_get_constraints(self): | ||
| """Test a list of constraits is returned by the method.""" |
* Address warnings issue * Add tests to single table + address feedback * Update err msg * Fix typo
* Address warnings issue * Add tests to single table + address feedback * Update err msg * Fix typo
Resolve #1171.