Skip to content

Conversation

BenChung
Copy link
Contributor

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

This extends SymbolicContinuousCallback to support the affect_neg and rootfind mechanisms provided by the underlying SciMLBase callback API. In particular, the semantics of affect_neg are mirrored into the symbolic domain (same as affect by default; disabled if nothing, and usable as either the equation or FunctionalAffect interface). The implementation of rootfind is somewhat more cursed; we cannot generate a single VectorContinuousCallback for different rootfind options, so instead generate several using the rootfind value as an equivalence class and then sort the resulting callbacks in order of rootfind. The alternative approach would be to generate a CallbackSet if the order violated the order of VectorContinousCallbacks (e.g. if we had a left then right then left rootfind selection), but this comes with a performance penalty. As a result, we have a choice between impacting performance and odd semantics; I'd appreciate feedback on what the right choice is here.

@BenChung
Copy link
Contributor Author

BenChung commented Jul 31, 2024

Stuff that I want to do for this commit:

  • Fully check compliance with the style guide.
  • Extend the narrative documentation that exploits the affect_neg and rootfinding side functionality
  • Write more extensive docstrings for the relevant methods & expose this into the public facing documentation.

@BenChung
Copy link
Contributor Author

BenChung commented Jul 31, 2024

I'm going to put off writing more extensive docstrings until after the API reference gets generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants