Skip to content

Refactor/direct collocation backend #189

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Mar 16, 2025
Merged

Refactor/direct collocation backend #189

merged 19 commits into from
Mar 16, 2025

Conversation

@andgoldschmidt
Copy link
Member

@andgoldschmidt andgoldschmidt commented Mar 13, 2025 •
edited
Loading

  • Updated all min time and sampling problem templates for kets and unitaries
  • Some bug fixes in fidelity defintions
  • Free time is no longer optional
  • Add final fidelity constraints using KnotPointConstraint from DirectTrajOpt.jl
  • Removed some piccolo options that were not being used (options are not tested well)
  • A handful of objectives are gone (temporarily?), leading to a few removed PTs
  • unrelated, remove density pt bc didn't have a chance to refactor and write its tests

Some potential outstanding issues:

  1. Minimum time problems are only well-behaved with fidelity constraint of 1.0
  2. Minimum time problems currently require the goals to be passed as an argument. This is because the trajectory stores the goal states, but not the subspaces. This should eventually be addressed because there's no reason to need to pass these around as args.
  3. Problem template testing should start taking advantage of shared code.

aarontrowbridge and others added 9 commits March 10, 2025 14:47
@aarontrowbridge
interface changes
89841c1
@aarontrowbridge
refactor: update density operator problem and coherent ket function f…
879c9de 
…or clarity and consistency
@aarontrowbridge
refactor: replace hardcoded integrator with parameterized ket_integra…
c0e35d5 
…tor in QuantumStateSmoothPulseProblem
@aarontrowbridge
refactor: simplify unitary smooth pulse problem tests and normalize i…
b1a0831 
…nfidelity loss calculation
@aarontrowbridge
refactor: improve logging options and enhance infidelity loss calcula…
8fac1f5 
…tion in unitary smooth pulse problem
@andgoldschmidt
prune piccolo options
89cb570
@aarontrowbridge
refactor: rename and simplify infidelity loss functions for improved …
e551bb7 
…clarity and consistency
@andgoldschmidt
refactor: add quantum constraints module and update problem templates…
3920ad9 
…, piccolo options for DTO
@andgoldschmidt
Merge branch 'refactor/direct-collocation-backend' of github.com:harm…
cac7c64 
…oniqs/QuantumCollocation.jl into refactor/direct-collocation-backend
@andgoldschmidt
Copy link
Member Author

andgoldschmidt commented Mar 13, 2025 •
edited
Loading

Tests pass locally with develop install of harmoniqs/DirectTrajOpt.jl#4

@andgoldschmidt andgoldschmidt requested review from aarontrowbridge and jack-champagne and removed request for aarontrowbridge March 13, 2025 08:12
@jack-champagne
Copy link
Member

jack-champagne commented Mar 13, 2025

Pulling this down this AM to test build

@jack-champagne
Copy link
Member

jack-champagne commented Mar 13, 2025

Everything looks great, I'm going to make a package addition this PM for DirectTrajOpt.jl into general. I am worried though that it will be flagged for similarity since IndirectTrajOpt.jl is also registered. But I believe we should be able to override that with a quick discussion

@andgoldschmidt
Copy link
Member Author

andgoldschmidt commented Mar 13, 2025

The issue with min time problem templates and the fidelity constraint has been resolved, it was an objective weight and an inequality ordering issue.

@jack-champagne
Copy link
Member

jack-champagne commented Mar 15, 2025

No more dev install, all tests pass (except for one that I marked as broken because it was not consistently passing. Ticket to fix has been made.)

jack-champagne
jack-champagne reviewed Mar 16, 2025
View reviewed changes
src/problem_templates/unitary_smooth_pulse_problem.jl
Comment on lines +208 to +209
@test_broken false
# @test final > initial
Copy link
Member

@jack-champagne jack-champagne Mar 16, 2025

Choose a reason for hiding this comment

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

This is the one marked broken for now.

Copy link
Member Author

@andgoldschmidt andgoldschmidt Mar 16, 2025

Choose a reason for hiding this comment

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

I'll have to check, I didn't do any work on the piccolo options constraints for this refactor, and don't remember adjusting. I'll have to look again when I'm on my laptop not mobile

Copy link
Member

@jack-champagne jack-champagne Mar 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, this has been flaky for a while - don't think this was induced by your work for this since you didn't touch PiccoloOptions and I saw this happening months ago

@jack-champagne jack-champagne merged commit 2210265 into main Mar 16, 2025
3 checks passed
@jack-champagne jack-champagne mentioned this pull request Mar 16, 2025
Closed
This was referenced Apr 15, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jack-champagne jack-champagne jack-champagne left review comments

@aarontrowbridge aarontrowbridge Awaiting requested review from aarontrowbridge

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@andgoldschmidt @jack-champagne @aarontrowbridge