-
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
Ensemble docs #437
Ensemble docs #437
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## smartsim-docs-refresh #437 +/- ##
=========================================================
+ Coverage 90.27% 90.47% +0.19%
=========================================================
Files 60 60
Lines 3857 3738 -119
=========================================================
- Hits 3482 3382 -100
+ Misses 375 356 -19 |
doc/ensemble.rst
Outdated
|
||
Notice that `perm_strategy="step"` which means values of the `params` key will be | ||
grouped into intervals and distributed across Ensemble members. Here there are two groups. Therefore, | ||
the Ensemble will have two ``Model`` members. |
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.
I had to read the code to be sure what step strategy did. Should we add a table to each strategy showing the expected groups?
doc/ensemble.rst
Outdated
------------------------------------------------------ | ||
In this example, we demonstrate how to instruct SmartSim to load | ||
an in-memory ML model into the database at ensemble runtime. It's | ||
important to note that in-memory ML models are supported for |
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.
-
"it's important to note..." could be a note/warning block
-
is it meant to say "in-memory ML models are supported for non-colo..." OR "in-memory ML models are supported ONLY for non-colo..."
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.
- Could we just say "standard" instead of "non-colo"? It feels more readable to avoid the negation and just say what we mean.
The closing blurb says standard:
When the ensemble is started via ``Experiment.start()``, the ML model will be loaded to the standard orchestrator that is launched prior to the start of the ensemble.
doc/ensemble.rst
Outdated
|
||
In this integration, we provide the following details: | ||
|
||
- `name`: "example_script" - key to store script under |
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.
typehints?
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.
Very small changes but otherwise I think we can merge soon. Thanks!
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!
365442f
into
CrayLabs:smartsim-docs-refresh
This Pr merges the Ensemble documentation into the feature branch