feat: add __str__ methods to the various part of the profiler options#1115
feat: add __str__ methods to the various part of the profiler options#1115carlsonp wants to merge 11 commits intocapitalone:devfrom
Conversation
taylorfturner
added
the
New Feature
taylorfturner
left a comment
taylorfturner
left a comment
There was a problem hiding this comment.
Yep, exactly how I was thinking about utilizing the __str__ magic method. I may recommend two things here, though:
- some variable name changes for clarity in the
__str__. We use options as a variable in throughout and you are usingoptionsin that for loop. Just for clarity of variable naming... maybe something along the line ofiter_optionwhen you are referencing in theforloop so its super explicit that it is related to theforloop - and adding the presets on the
ProfilerOptions.__str__return implementation for clarity
* add downloads tile (capitalone#1085) * Replace snappy with cramjam * Delete test_no_snappy --------- Co-authored-by: Taylor Turner <taylorfturner@gmail.com>
carlsonp
force-pushed
the
profile-options-str
branch
from
2899858 to
dbc9a80
Compare
* Fix dask_expr * Keras and Tensorflow version fix * Keras and Tensorflow version fix * Fix keras bug
|
@carlsonp you should be good to rebase onto |
|
Thanks, I'll get to it next week. Fighting off a bug.
…On Fri, Mar 22, 2024, 2:51 PM Taylor Turner ***@***.***> wrote:
@carlsonp <https://github.com/carlsonp> you should be good to rebase onto
dev now
—
Reply to this email directly, view it on GitHub
<#1115 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAI5DNM6YUBHLUBLCTAKNNDYZSDTNAVCNFSM6AAAAABEU57M7GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJVHAYDGMRQGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…not the options variable
carlsonp
force-pushed
the
profile-options-str
branch
from
dbc9a80 to
cb15c63
Compare
|
Rebased |
|
@carlsonp yeah, I like the route you are going. Once you add unit tests, just tag me and I'll take another look at it. Cheers! |
@taylorfturner Can you please provide a suggested starting point for which file to add the unit tests? |
| def __str__(self) -> str: | ||
| """ | ||
| Return a human friendly consumable output in string form. | ||
|
|
||
| :return: str of the option presets and properties | ||
| :rtype: str | ||
| """ | ||
| return f"Presets: {str(self.presets)}\n \ | ||
| {str(self.structured_options)}\n \ | ||
| {str(self.unstructured_options)}" | ||
|
|
There was a problem hiding this comment.
this makes sense here and you would test in test_profiler_options.py
Yes, indeed! I would actually move some of the |
|
@carlsonp you'll want a rebase here too onto |
| def __str__(self) -> str: | ||
| """ | ||
| Return a human friendly consumable output in string form. | ||
|
|
||
| :vartype dict_string: dict | ||
| :return: str of the option properties | ||
| :rtype: str | ||
| """ | ||
| dict_string: dict = {"CategoricalOptions": []} | ||
| for iter_option in [ | ||
| a | ||
| for a in dir(self) | ||
| if not a.startswith("__") and not callable(getattr(self, a)) | ||
| ]: | ||
| dict_string["CategoricalOptions"].append( | ||
| {str(iter_option): str(getattr(self, iter_option))} | ||
| ) | ||
| return json.dumps(dict_string, indent=4) | ||
|
|
There was a problem hiding this comment.
might be good to find a way to abstract this a bit more so this ends up in BaseOption 90%+ of this code is repeat just with string changes: so I think there is room to make this DRY-er
4 participants
How about this as a starting point for adding some helpful printing of Data Profiler options?