Skip to content

Conversation

@gennadiryan
Copy link
Member

Fixes harmoniqs/QuantumCollocation.jl#157 (broken following deprecation of QuantumCollocationCore); other PR will build on this to serialize optimizer state as well.

@andgoldschmidt
Copy link
Member

@BBhattacharyya1729 does this capture the full spirit of your local callbacks reimplementation?

@gennadiryan gennadiryan force-pushed the feature/ipopt-callbacks branch from be21514 to 7f01474 Compare October 14, 2025 08:10
@gennadiryan
Copy link
Member Author

@jack-champagne Let me know if you like the "take 2" factory pattern better (I included usage examples below; waiting to add my tests once we decide which way to go with this); it seems a bit more flexible wrt writing new callbacks/composing existing callbacks. I had initially written the individual callbacks (_callback_update_trajectory, _callback_rollout_fidelity, etc.) as bare functions which would take their parameters as kwargs (e.g. you pass the problem, system, fidelity threshold, other parameters the callback depends on etc. as kwargs to callback_factory), but that seemed a bit messy because of (a) risk of name collision and more importantly (b) kwargs are not typed in Julia (behind the scenes, type signatures on kwargs are ignored; they're just treated as annotations by the compiler). As a result I reverted to using a factory pattern for everything; imo it's a lot cleaner now, but curious about your thoughts on this.

Copy link
Member

@jack-champagne jack-champagne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor changes! looking good to me

@gennadiryan gennadiryan self-assigned this Oct 24, 2025
Copy link
Member

@jack-champagne jack-champagne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fantastic! let's get this merged. @gennadiryan bump the project toml version number first before you merge.

@gennadiryan gennadiryan merged commit 910d9d0 into main Oct 24, 2025
3 checks passed
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.

4 participants