Reverse transform constraints later in the DataProcessor
#1511
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #1511 +/- ##
=======================================
Coverage 96.28% 96.28%
=======================================
Files 49 49
Lines 3932 3934 +2
=======================================
+ Hits 3786 3788 +2
Misses 146 146
☔ View full report in Codecov by Sentry. |
fealho
requested review from
amontanez24 and
frances-h
and removed request for
a team
fealho
force-pushed
the
issue-1476-change-constraint-order-during-reverse
branch
from
f1677bb
to
4710b7b
Compare
| for constraint in reversed(self._constraints_to_reverse): | ||
| reversed_data = constraint.reverse_transform(reversed_data) |
There was a problem hiding this comment.
would it work if you just did sampled_columns = constraint.reverse_transform(sampled_columns)?
There was a problem hiding this comment.
Sorry, I don't get what you mean, you can't apply reverse_transform to sampled_columns, since that's a list.
There was a problem hiding this comment.
Yeah you're right, I got confused. Should we just set sampled_columns to be reversed_data.columns on line 725 instead of extending. Otherwise that list might contain columns that were created by constraints that we don't want
There was a problem hiding this comment.
That was actually the first thing I tried. It doesn't work because sampled_columns has the keys, which reversed_data.columns doesn't have.
Your concern is actually not a problem because of line 731, it removes any columns not seen in the metadata.I originally had an extra two lines to remove these extra columns directly, for clarity, but I decided to remove them since they were unnecessary.
There was a problem hiding this comment.
That makes sense. Can you edit the comments before line 731 to add that extra columns will also be dropped?
| for constraint in reversed(self._constraints_to_reverse): | ||
| reversed_data = constraint.reverse_transform(reversed_data) |
There was a problem hiding this comment.
That makes sense. Can you edit the comments before line 731 to add that extra columns will also be dropped?
Resolve #1476.