Skip to content

Conversation

@katxiao
Copy link
Contributor

@katxiao katxiao commented Sep 15, 2022

Resolves #207

@katxiao katxiao marked this pull request as ready for review September 21, 2022 17:08
@katxiao katxiao requested a review from a team as a code owner September 21, 2022 17:08
@katxiao katxiao requested review from pvk-developer and removed request for a team September 21, 2022 17:08
@katxiao katxiao changed the title Add synthetic uniqueness single table metric Add new row synthesis single table metric Sep 22, 2022
@katxiao katxiao force-pushed the issue-207-synthetic-uniqueness branch from 0be9683 to 376bec4 Compare September 22, 2022 18:19
Comment on lines +92 to +93
if isinstance(columns, set):
columns = list(columns)
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but not sure what columns can be:
You can always cast to list and this will be one dimensional:

In [1]: columns_list = ['a', 'b', 'c']
In [2]: columns_set = {'a', 'b', 'c'}
In [3]: columns_tuple = ('a', 'b', 'c')

In [4]: list(columns_list)
Out[4]: ['a', 'b', 'c']

In [5]: list(columns_set)
Out[5]: ['b', 'c', 'a']

In [6]: list(columns_tuple)
Out[6]: ['a', 'b', 'c']

If columns can be a str then don't do this as it will split it in letters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should always be a list or a set.

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2022

Codecov Report

Base: 70.18% // Head: 73.60% // Increases project coverage by +3.41% 🎉

Coverage data is based on head (b17f299) compared to base (62e6b73).
Patch coverage: 100.00% of modified lines in pull request are covered.

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
+ Coverage   70.18%   73.60%   +3.41%     
==========================================
  Files          70       72       +2     
  Lines        2482     2811     +329     
==========================================
+ Hits         1742     2069     +327     
- Misses        740      742       +2     
Impacted Files Coverage Δ
sdmetrics/multi_table/__init__.py 100.00% <ø> (ø)
sdmetrics/multi_table/multi_single_table.py 82.71% <100.00%> (+0.89%) ⬆️
sdmetrics/reports/single_table/plot_utils.py 100.00% <100.00%> (ø)
sdmetrics/single_column/__init__.py 100.00% <100.00%> (ø)
sdmetrics/single_column/statistical/__init__.py 100.00% <100.00%> (ø)
...etrics/single_column/statistical/range_coverage.py 100.00% <100.00%> (ø)
sdmetrics/single_table/__init__.py 100.00% <100.00%> (ø)
sdmetrics/single_table/multi_single_column.py 90.62% <100.00%> (+0.46%) ⬆️
sdmetrics/single_table/new_row_synthesis.py 100.00% <100.00%> (ø)
... and 4 more

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@pvk-developer pvk-developer left a comment

Choose a reason for hiding this comment

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

This looks good now! :shipit:

@katxiao katxiao merged commit 315e34a into master Sep 23, 2022
@katxiao katxiao deleted the issue-207-synthetic-uniqueness branch September 23, 2022 21:01
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.

Add NewRowSynthesis Metric

4 participants