Skip to content
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 frequencies as optional CRAB parameter #18

Merged
merged 13 commits into from
May 6, 2024

Conversation

flowerthrower
Copy link
Contributor

This PR introduces variable Fourier frequencies for the CRAB algorith.
So far they were fixed. This is still the default behaviour and all additional parameters are optional (no breaking changes).
Recently a user requested to provide initial starting values for all parameters. With this PR it can be done in the new QOC package.

@coveralls
Copy link

coveralls commented Mar 3, 2024

Pull Request Test Coverage Report for Build 8959043767

Details

  • 30 of 32 (93.75%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.6%) to 69.557%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/qutip_qtrl/pulsegen.py 14 16 87.5%
Totals Coverage Status
Change from base Build 8063466154: 0.6%
Covered Lines: 3313
Relevant Lines: 4763

💛 - Coveralls

@hodgestar
Copy link
Contributor

@flowerthrower Not sure if you intended this for review already. Could you also update the docstrings, add a test and ideally some sort of example to the documentation itself (i.e. under docs/)?

@flowerthrower flowerthrower marked this pull request as draft March 11, 2024 19:17
@flowerthrower
Copy link
Contributor Author

flowerthrower commented Mar 11, 2024

Sorry @hodgestar, I did not intend to mark this as "ready for review" yet: I am not finished working on it.

@flowerthrower
Copy link
Contributor Author

flowerthrower commented Apr 11, 2024

At this point of time this addition is mostly targeted to make qtrl accessible for qoc.
I designed the change to not interfere with anything existing in qtrl.
To fully integrate this option into qtrl would add considerable effort.
For the time being I would rather point users to use qoc to access this feature.

Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

I'm not sure the current solution makes much sense. The frequencies are only present in PulseGenCrabFourier but the fix_freqs option appears on the base PulseGenGran class. Can we move it to the PulseGenCrabFourier class?

It would be good to add a test for the new feature too.

@flowerthrower
Copy link
Contributor Author

Thank you @hodgestar for your feedback. I have incorporated your suggestions. Let me know if there is anything else!

@flowerthrower flowerthrower marked this pull request as ready for review April 17, 2024 06:54
@flowerthrower
Copy link
Contributor Author

Dear @ajgpitch, dear @hodgestar.
Could you please verify the recent additions I made?
I want to merge this PR to update the QOC dependency soon.
Thank you guys!

@ajgpitch
Copy link
Member

ajgpitch commented May 1, 2024

This all seems fine to me, but as @hodgestar has made suggestions (which seem to have been implemented), then best if he approves too.

@flowerthrower
Copy link
Contributor Author

Some tests are currently failing due to a qutip@master Cython version error.

@flowerthrower flowerthrower force-pushed the patrick/parameterize_crab branch from 476c300 to ac1f655 Compare May 4, 2024 10:42
Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

Two more small suggestions made.

@hodgestar
Copy link
Contributor

Some tests are currently failing due to a qutip@master Cython version error.

I reran the tests but the error is still the same. Noting the error message here:

ERROR: Some build dependencies for git+https://github.com/qutip/qutip.git@master conflict with the backend dependencies: cython==3.0.2 is incompatible with cython>=0.29.20,<3.0.0; python_version<='3.9'.

And indeed there is a Cython version discrepancy between the build and install requirements for Python 3.9 in the current qutip master branch:

Install: https://github.com/qutip/qutip/blob/320996bfda3a2eb361eb36cac4208990266b97fe/setup.cfg#L52
Build: https://github.com/qutip/qutip/blob/320996bfda3a2eb361eb36cac4208990266b97fe/pyproject.toml#L7

Note that the build requirements also seem to be duplicated in setup.cfg. Probably worth a least a comment in the file if the duplication is still needed.

@Ericgig Could you perhaps help @flowerthrower sort this out?

Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

I'm approving these changes, but let's get CI running and passing before actually merging it so that we've at least had a successful test run.

@hodgestar hodgestar changed the title Add frequenciess as optional CRAB parameter Add frequencies as optional CRAB parameter May 6, 2024
@hodgestar
Copy link
Contributor

CI tests are now passing. Thansk, @Ericgig.

@hodgestar hodgestar merged commit 31ec5fa into qutip:master May 6, 2024
7 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