-
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
Update add_ml_model and add_script #304
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #304 +/- ##
===========================================
- Coverage 87.44% 86.96% -0.49%
===========================================
Files 59 59
Lines 3481 3482 +1
===========================================
- Hits 3044 3028 -16
- Misses 437 454 +17
|
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.
Two quick requests tangentially related to this PR, but otherwise lgtm!!
smartsim/entity/model.py
Outdated
@@ -462,7 +468,9 @@ def add_function( | |||
:type script_path: str, optional |
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.
Looks like :param script:
and :param script_path:
are copy/paste errors from back in the day. Can you update these while you are here
Thanks for the great feedback. I'm going to move this PR to "draft" because I was looking at the code more and see that the tests for these functions were not being tested on GPU for WLM tests when device was set to GPU. I'll address your feedback and a few other things and change the status back when it is ready for review. |
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.
Looks great! A couple of small type errors, a couple quick changes to the tests, and couple of questions for follow up work outside of this PR, but otherwise looks good to me!!
4c6d8f3
to
a274906
Compare
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!! Thanks for the hard work on this one!!
9284dae
to
f169f74
Compare
a59ff32
to
af38806
Compare
This PR addresses incorrect or inconsistent behavior in the
Model.add_ml_model()
,Model.add_script()
andModel.add_function()
API. The following changes are included in this PR:lo
instead ofipogif0