-
Notifications
You must be signed in to change notification settings - Fork 414
Allow default transformers to have custom arguments #2431
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2431 +/- ##
==========================================
+ Coverage 98.58% 98.60% +0.01%
==========================================
Files 59 59
Lines 6161 6167 +6
==========================================
+ Hits 6074 6081 +7
+ Misses 87 86 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ce0c294 to
9c607a9
Compare
| @staticmethod | ||
| def _get_transformer_kwargs(transformer): | ||
| args = inspect.getfullargspec(transformer.__init__).args[1:] | ||
| return { | ||
| key: getattr(transformer, key) | ||
| for key in args | ||
| if key != 'model_missing_values' and hasattr(transformer, key) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method doesn't seem to belong here. We are trying to avoid creating static methods overall (following our standard). I feel like this method belongs more to RDT than the DataProcessor class. If we changed something, lets say, we deprecate one more argument, this change won't propagate here. However, if we were to add this to rdt we would require a new release etc.. and that is why I would like to suggest the following:
- Move this function to the
_utils.pyinsdv(if possible). - File an issue in
RDTregarding adding this functionality to the transformers. - File another issue in
SDVto remove this utility function and use the transformer's method instead.
07215cf to
ec3d3e3
Compare
ec3d3e3 to
144e716
Compare
This PR updates the data processor so that parameters used to instantiate the default transformer for an sdtype are used to create additional instances of the transformer.