-
-
Notifications
You must be signed in to change notification settings - Fork 553
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
Issue 3530 custom termination #3596
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
Thanks, looks good!
Not for this issue, but the mixed step
/string definitions of experiments is a little weird. I think users like the string experiments and dropping them would be a mistake.
Yeah there are good reasons to keep both, what are you suggesting? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3596 +/- ##
========================================
Coverage 99.58% 99.59%
========================================
Files 257 258 +1
Lines 20708 20755 +47
========================================
+ Hits 20623 20670 +47
Misses 85 85 ☔ View full report in Codecov by Sentry. |
Nothing for now, but I'm just conscious it may be confusing for new users to see both ways of doing things within the same experiment definition. |
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, happy to merge once coverage and tests are fixed! About the interface, I think it should be OK to maintain the string option for a subset of functionality.
Description
Adds ability to create custom termination events, using any variable, for an experiment step
First part of #3530 (second part is using custom steps, not just terminations)
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run
(or$ nox -s pre-commit
) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all
(or$ nox -s tests
)$ python run-tests.py --doctest
(or$ nox -s doctests
)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick
(or$ nox -s quick
).Further checks: