[SDV 2.0] Add high_cardinality flag to ordinal and categorical column metadata - #2991
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v2.0.0 #2991 +/- ##
=======================================
Coverage 97.92% 97.92%
=======================================
Files 66 66
Lines 7754 7754
=======================================
Hits 7593 7593
Misses 161 161
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This Pull Request is not linked to an issue. To ensure our community is able to accurately track resolved issues, please link any issue that will be closed by this PR! |
| 'range_values', | ||
| 'high_cardinality', |
There was a problem hiding this comment.
If we add high_cardinality here, we shouldn't need to pop it from the parameters in the the _get_ordinal_transformer and _get_categorical_transformer methods. Also, range_values should not be in this list, since we do want the range_values when instantiating ordinal transformers.
There was a problem hiding this comment.
Yes that's a good point thanks
Addressed in 3b15105
| table_str = f" for table '{table_name}'" if table_name else '' | ||
| sys.stdout.write(f'\nDetecting primary key{table_str}:\n') | ||
| _print_primary_key_detection(chosen_pk, sdtype_updated, pii_removed) | ||
| _print_primary_key_detection(chosen_pk) |
There was a problem hiding this comment.
Now that all the detection results are printed together at the end, I don't think we need to mention when the sdtype was updated anymore.
Before, it made sense because the output was printed during detection. Now it can be confusing to see sdtype='id' (updated to 'id') when we're already showing the final state.
Let me know if it makes sense this way.
SDV/sdv/metadata/_single_table.py
Line 852 in 9b7c327
|
This Pull Request is not linked to an issue. To ensure our community is able to accurately track resolved issues, please link any issue that will be closed by this PR! |
| from sdv.metadata import Metadata | ||
|
|
There was a problem hiding this comment.
Why are we moving this import statement?
There was a problem hiding this comment.
It was meant to fix the integration tests failing on SDV-Enterprise. When running the tests with multiple workers using pytest-xdist, we were hitting partially initialized module errors: https://github.com/datacebo/SDV-Enterprise/actions/runs/35715624169/job/106706319805
However, I ended up going with another solution: import sdv in the SDV-Enterprise conftest.py, so it is fully initialized before xdist starts handling worker warnings. This avoids the partial import issue for the workers.
Let me know if this works.
|
This Pull Request is not linked to an issue. To ensure our community is able to accurately track resolved issues, please link any issue that will be closed by this PR! |
4 participants
Resolve #2990
86bc1nxb8