-
Notifications
You must be signed in to change notification settings - Fork 41
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
Adding verbose argument to estimate_cost and real_cost #1207
Conversation
fd365b4
to
ad7e19a
Compare
CHANGELOG.md
Outdated
@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
### Added | |||
- Support for multiple frequencies in `output_monitors` in `adjoint` plugin. | |||
- GDSII export functions to `Simulation`, `Structure`, and `Geometry`. | |||
- 'verbose' argument to `estimate_cost` and `real_cost` functions such that the cost is logged if `verbose==True` (default). Additional helpful messages may also be logged. |
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.
backslash quotes?
if batch_cost is not None and batch_cost > 0: | ||
console.log(f"Maximum FlexCredit cost: {batch_cost:1.3f} for the whole batch.") | ||
else: | ||
console.log("Could not get estimated batch cost!") |
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.
maybe say why (eg it might not have finished computing, wait and try again?)
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.
Actually when it comes to estimated cost, it should always work now. This is probably legacy code. This statement should not be reachable since if something goes wrong, the estimate cost should at least error. So yeah I don't really know of a case when this can be reached and so don't know how to improve the message - but may just leave it just in case?
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 good. I suppose we are keeping the original naming of "flex unit" instead of "flex credit"?
I assume you mean for the variable names? Yeah this is a bit annoying to change, it involves both changing our python code and (mostly) the api endpoints and the server code... |
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 good. I just was confused what "Note: the task triggered a minimum flex credit cost due to fast early shutoff." means exactly: 0.025 min cost or 1/10 of estimated cost. Maybe this can be clarified somehow?
more detailed messages cleaning up some docstrings
ad7e19a
to
f8c7ddf
Compare
It refers to the 1/10 of estimated cost. I don't want to hard-code that as it is complicated though. For example, we only pro-rate the part that is due to time stepping; not the part that is due to e.g. field projections, or in the future, mode solver. Furthermore, the 1/10 factor may change, and the message will become stale if it does. I updated the message to
and then also updated our how are simulations billed FAQ that we can refer people to for more details. There, I put the current 1/10 factor since we can change the docs if that changes: @tomflexcompute will that change automatically propagate to the knowledge base, or does it have to happen manually? |
@tomflexcompute any comments on this or my FAQ update before I merge (also see question above on propagating FAQ to knowledge base)? |
Looks good to me. The FAQs will be propagating to the Learning Center and Emerson is working on it. Eventually we will combine the FAQs in our docs and knowledge base and open a FAQs section in the Learning Center. Then the FAQs in both places can be removed or at least synced with the Learning Center. |
more detailed messages
cleaning up some docstrings
fixes #1161
Docs PR specifically related to estimate_cost and real_cost updated handling.