-
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
Add dn based output trigger #1210
Conversation
I can't tell whether the macOS CI failure is related or not:
It looks like some of the output files don't exist, so it might be. |
I think the updated handling of ensuring a last time based output is written might be broken (or not behaving as before any more). |
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.
Assuming we can make tests pass, LGTM. 👍 for deleting dead code.
The failed tests were related to a change in logic for setting |
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.
Typo in docs. Otherwise LGTM
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.
Other than the minor docs typo LGTM
PR Summary
What is says on the can (plus removed dead vtk code as I don't think we'll ever implement/reactivate it).
Still need
PR Checklist