-
Notifications
You must be signed in to change notification settings - Fork 37
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
Remove Pandas from experiment summary #116
Remove Pandas from experiment summary #116
Conversation
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.
hey Matt! great stuff on the PR!
A couple comments
theres a test in on_wlm/test_simple_entity_launch.py
that tests the summary function and will need to be changed. In addition, we should add another test in the test_experiment.py
file to test another simple run.
…ty_launch.py summary test
Merge remote-tracking branch 'upstream/develop' into remove_np_from_experiment_summary
Changes:
|
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.
Small changes! Otherwise looking great!
@@ -467,35 +467,35 @@ def summary(self): | |||
The summary will show each instance that has been | |||
launched and completed in this ``Experiment`` | |||
|
|||
:return: pandas Dataframe of ``Experiment`` history | |||
:rtype: pd.DataFrame | |||
:return: tabulate string of ``Experiment`` history |
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.
Lets allow format
as a parameter here and pass to the tabulate function. Also should add in https://github.com/astanin/python-tabulate#table-format to the docstring so that people know what options are available.
smartsim/experiment.py
Outdated
job.history.returns[run], | ||
] | ||
) | ||
return tabulate(values, headers, showindex=True, tablefmt="plain") |
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.
lets set the default to github
. so the added parameter will be a keyword arg like format=github
and then that input parameter will be passed here.
Changes:
|
…t_summary Bring up to date with upstream
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.
LGTM, lets chat before you merge!
Experiment.summary
method by removing usage of a pandas dataframe and instead utilizing tabulate to format the summary table