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.

Sign up for GitHub

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

Fix bug with null replication metrics when row is all null #706

Merged
merged 6 commits into from
Nov 8, 2022
Merged

Fix bug with null replication metrics when row is all null #706

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bee16d3
Fix bug when row is all null
tonywu315 Nov 4, 2022
7644b79
Improve test
tonywu315 Nov 4, 2022
a3b0a16
Remove unnecessary type
tonywu315 Nov 4, 2022
1d94e12
Merge branch 'main' into row_null_bug
tonywu315 Nov 7, 2022
f6bcf32
Merge branch 'main' into row_null_bug
tonywu315 Nov 8, 2022
00ee95a
Improve efficiency
tonywu315 Nov 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion dataprofiler/profilers/profile_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2240,7 +2240,20 @@ def _update_null_replication_metrics(self, clean_samples: Dict) -> None:
:param clean_samples: input cleaned dataset
:type clean_samples: dict
"""
data = pd.DataFrame(clean_samples).apply(pd.to_numeric, errors="coerce")
data = pd.DataFrame(clean_samples)

# If the last row is all null, then add rows to the data DataFrame
max_null_index = max(
[max(i) for i in getattr(self._profile[0], "null_types_index").values()],
Copy link
Contributor

Choose a reason for hiding this comment

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

self._profile[0] Right now this is getting the first profile every time is this correct?

Why do we need to add rows to the df?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the last row(s) is all null, then the maximum null index of any profile should be the index of the last row.

Copy link
Contributor

@taylorfturner taylorfturner Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

  • in this test case, len(self._profile) is 3 and only the zero-th index has values.
getattr(self._profile[0], "null_types_index").values()
dict_values([{2, 4}])
getattr(self._profile[1], "null_types_index").values()
dict_values([])
getattr(self._profile[2], "null_types_index").values()
dict_values([])

@tonywu315 is this always the case though? are we sure we can always just pull the zero-th profile in the self._profile list?

default=0,
)
if max_null_index > data.index.max():
data.loc[max_null_index] = {}

# Fill in missing rows with NaN and convert types to numeric
data = data.reindex(range(data.index.max() + 1), fill_value=np.nan).apply(
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be really expensive for large data. Is there another way around this?

Copy link
Contributor

Choose a reason for hiding this comment

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

What specific error is requiring this change / where?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 good catch @JGSweets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a row is all null, the dataframe will be missing a row, which causes an error on the line
sum_null = data.iloc[null_indices, data.columns != col_id].sum().to_numpy().
reindex fills in these missing rows with nan.

Copy link
Contributor

Choose a reason for hiding this comment

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

The .reindex() though when done on an entire dataset could be very costly in terms of runtime; so that is the concern around this operation that is being added to the process.

Copy link
Contributor

Choose a reason for hiding this comment

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

null_indices + non-complete null rows?

Copy link
Contributor

Choose a reason for hiding this comment

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

or even better potentially would be null_indices - complete null rows (theoretically there should be less completely null rows)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another way would be to reindex a single column in clean_samples and then combine all of them into data, which should also fix the bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would work too

Copy link
Contributor

@JGSweets JGSweets Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Any way we can do it w/o re-indexing? I think that would be the ideal state.

pd.to_numeric, errors="coerce"
)

get_data_type = lambda profile: profile.profiles[ # NOQA: E731
"data_type_profile"
Expand Down
15 changes: 15 additions & 0 deletions dataprofiler/tests/profilers/test_profile_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2081,6 +2081,21 @@ def test_null_replication_metrics_calculation(self):
np.testing.assert_array_almost_equal([[np.nan], [18]], column["class_sum"])
np.testing.assert_array_almost_equal([[np.nan], [9]], column["class_mean"])

# Test with all null in a row
data_4 = pd.DataFrame(
[[10, 20], [9999999, 9999999], [30, 9999999], [9999999, 9999999]]
)

profiler = dp.StructuredProfiler(data_4, options=profile_options)
report = profiler.report()

self.assertTrue("null_replication_metrics" in report["data_stats"][0])
column = report["data_stats"][0]["null_replication_metrics"]

np.testing.assert_array_almost_equal([0.5, 0.5], column["class_prior"])
np.testing.assert_array_almost_equal([[20], [0]], column["class_sum"])
np.testing.assert_array_almost_equal([[10], [0]], column["class_mean"])

def test_column_level_invalid_values(self):
data = pd.DataFrame([[1, 1], [9999999, 2], [3, 3]])

Expand Down