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

Migrate adding and validating constraints to BaseSynthesizer #1163

Closed
amontanez24 opened this issue Jan 6, 2023 · 0 comments
Closed

Migrate adding and validating constraints to BaseSynthesizer #1163

amontanez24 opened this issue Jan 6, 2023 · 0 comments
Assignees
Labels
feature request Request for a new feature
Milestone

Comments

@amontanez24
Copy link
Contributor

amontanez24 commented Jan 6, 2023

Problem Description

As a user, it would be nice if the metadata used through the SDV ecosystem was the same. For some tools, constraints are irrelevant and so having them in the metadata is confusing.

In order to make metadata consistent throughout our ecosystem, we want to separate constraints from it.

Acceptance criteria

  • Move all functionality involving constraints in the metadata into the BaseSynthesizer class. This includes
  • Instead of add_constraint, create a new add_constraints method
    • Parameters
      • constraints - List of constraints described as dicts in the following format:
        {'constraint_class': NAME, 'constraint_parameters': {'param_1': ..., 'param_2': ...}}
    • Error Handling: Perform the same validation & error handling that we are already doing. Note that we always have access to the metadata because it is required when instantiating the synthesizer.
      • If the user has already fit the synthesizer, warn them that they need to refit it.
        Warning: For these constraints to take effect, please refit the synthesizer using 'fit'
    • If called multiple times, continue to add to the existing list of constraints.

Expected behavior

age_is_positive = {
  'constraint_class': 'Positive',
  'constraint_parameters': {
    'column': 'age',
    'strict_boundaries': True
  }
}

synthesizer.add_constraints(constraints=[age_is_positive])

Additional context

  • On top of moving the constraint logic to the BaseSynthesizer there are some other key changes. The new method for adding constraints take in a list instead of an individual constraint dictionary. Additionally, the format of the constraint dictionary is different as the parameters are now in a nested dict. This means that some logic around the validation code will likely need to change.
  • The metadata_upgrader will no longer need to update constraints. The code for converting constraints can stay but remove the conversion from convert_metadata
@amontanez24 amontanez24 added the feature request Request for a new feature label Jan 6, 2023
@amontanez24 amontanez24 added this to the 1.0.0 milestone Jan 6, 2023
@amontanez24 amontanez24 added the under discussion Issue is currently being discussed label Jan 6, 2023
@amontanez24 amontanez24 removed the under discussion Issue is currently being discussed label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for a new feature
Projects
None yet
Development

No branches or pull requests

2 participants