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.

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

feat: add __str__ methods to the various part of the profiler options #1115

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

carlsonp
Copy link
Contributor

How about this as a starting point for adding some helpful printing of Data Profiler options?

import dataprofiler as dp
profile_options = dp.ProfilerOptions()
print(profile_options)

@carlsonp carlsonp requested a review from a team as a code owner March 13, 2024 21:27
@carlsonp carlsonp marked this pull request as draft March 13, 2024 21:35
@taylorfturner taylorfturner added the New Feature A feature addition not currently in the library label Mar 14, 2024
Copy link
Contributor

@taylorfturner taylorfturner left a comment

Choose a reason for hiding this comment

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

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 using options in that for loop. Just for clarity of variable naming... maybe something along the line of iter_option when you are referencing in the for loop so its super explicit that it is related to the for loop
  • 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 <[email protected]>
@taylorfturner
Copy link
Contributor

You'll want to rebase onto dev... had some other PRs from @gliptak merge into dev this morning.

Have you done rebases before, @carlsonp?

@carlsonp carlsonp marked this pull request as ready for review March 14, 2024 14:50
abajpai15 and others added 2 commits March 22, 2024 15:24
* Fix dask_expr

* Keras and Tensorflow version fix

* Keras and Tensorflow version fix

* Fix keras bug
@taylorfturner
Copy link
Contributor

@carlsonp you should be good to rebase onto dev now

@carlsonp
Copy link
Contributor Author

carlsonp commented Mar 22, 2024 via email

@carlsonp
Copy link
Contributor Author

Rebased

@taylorfturner
Copy link
Contributor

@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!

@carlsonp
Copy link
Contributor Author

carlsonp commented Apr 24, 2024

@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?

@taylorfturner taylorfturner changed the base branch from 0.10.9-dev to dev June 7, 2024 18:25
Comment on lines +1842 to +1852
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)}"

Copy link
Contributor

Choose a reason for hiding this comment

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

this makes sense here and you would test in test_profiler_options.py

@taylorfturner
Copy link
Contributor

@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?

Yes, indeed! I would actually move some of the __str__ methods you overwrite in this PR into base_options.py. Then I think your testing would much more simple. Ultimately this would allow you to test main implementation in test_base_option.py then you could build out small scenarios in the other option test files in tests/profilers/profiler_options/...

@taylorfturner
Copy link
Contributor

@carlsonp you'll want a rebase here too onto dev

Comment on lines +971 to +989
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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Feature A feature addition not currently in the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants