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

Set HyperTransformer configuration manually #995

Merged

Conversation

pvk-developer
Copy link
Member

@pvk-developer pvk-developer commented Sep 6, 2022

Resolves #982

@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2022

Codecov Report

Merging #995 (4dfad77) into master (7a70db6) will increase coverage by 0.21%.
The diff coverage is 91.48%.

❗ Current head 4dfad77 differs from pull request most recent head 24e4314. Consider uploading reports for the commit 24e4314 to get more accurate results

@@            Coverage Diff             @@
##           master     #995      +/-   ##
==========================================
+ Coverage   70.24%   70.46%   +0.21%     
==========================================
  Files          38       38              
  Lines        2850     2878      +28     
==========================================
+ Hits         2002     2028      +26     
- Misses        848      850       +2     
Impacted Files Coverage Δ
sdv/metadata/table.py 82.06% <78.57%> (-0.35%) ⬇️
sdv/tabular/copulagan.py 91.17% <90.90%> (+1.17%) ⬆️
sdv/constraints/base.py 97.26% <100.00%> (+0.31%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@pvk-developer pvk-developer changed the title Issue 982 set hypertransformer config manually Set HyperTransformer configuration manually Sep 6, 2022
@pvk-developer pvk-developer marked this pull request as ready for review September 6, 2022 13:59
@pvk-developer pvk-developer requested a review from a team as a code owner September 6, 2022 13:59
@pvk-developer pvk-developer requested review from sdv-team, amontanez24 and katxiao and removed request for a team and sdv-team September 6, 2022 13:59
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 mostly looks good to me. I just have a question about where the metadata is being used and why it didn't work before

for name, dtype in dtypes.items():
dtype = np.dtype(dtype).kind
field_metadata = self._fields_metadata.get(name, {})
transformer_template = field_metadata.get(
Copy link
Contributor

Choose a reason for hiding this comment

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

does this take the metadata into account at all? I'm trying to figure out why it wasn't before and what has changed now. For example in the student_placements demo it wasn't making the duration column categorical even though the metadata said it was

Copy link
Member Author

Choose a reason for hiding this comment

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

Old code was reading properly the dtype but was printing the wrong config which leaded to confusions as the transformers were updated after the detect. Now both sdtypes and transformers are being set corresponding to the dtype from the metadata with the set_config method.

@pvk-developer pvk-developer force-pushed the issue-982-set-hypertransformer-config-manually branch from 00c9a77 to 4dfad77 Compare September 6, 2022 22:27
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.

Looks good! :shipit:

Copy link
Contributor

@katxiao katxiao left a comment

Choose a reason for hiding this comment

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

lgtm, with a question


if transformer_template is None:
sdtypes[name] = self._DTYPES_TO_TYPES.get(dtype, {}).get('type', 'categorical')
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a unit test that checks this case (when transformer_template is None)?

@pvk-developer pvk-developer merged commit 0af85b8 into master Sep 7, 2022
@pvk-developer pvk-developer deleted the issue-982-set-hypertransformer-config-manually branch September 7, 2022 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set HyperTransformer config manually, based on Metadata if given
4 participants