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

RL algorithm implementation #19

Merged
merged 16 commits into from
Oct 4, 2024
Merged

RL algorithm implementation #19

merged 16 commits into from
Oct 4, 2024

Conversation

LegionAtol
Copy link
Contributor

No description provided.

Copy link
Collaborator

@flowerthrower flowerthrower left a comment

Choose a reason for hiding this comment

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

This already looks really good.
Most of my comments are small things that can be done in one line of code.
Please make sure to remove the TODO comments and make all tests pass.
All in all, great work that will be ready for merging soon.

src/qutip_qoc/pulse_optim.py Outdated Show resolved Hide resolved
src/qutip_qoc/pulse_optim.py Outdated Show resolved Hide resolved
src/qutip_qoc/pulse_optim.py Outdated Show resolved Hide resolved
tests/test_result.py Show resolved Hide resolved
src/qutip_qoc/_rl.py Outdated Show resolved Hide resolved
src/qutip_qoc/_rl.py Outdated Show resolved Hide resolved
src/qutip_qoc/_rl.py Outdated Show resolved Hide resolved
src/qutip_qoc/_rl.py Outdated Show resolved Hide resolved
src/qutip_qoc/_rl.py Outdated Show resolved Hide resolved
tests/test_result.py Show resolved Hide resolved
… train early, removed update_solver(), use of var_time = True supported, other minor fixes
Copy link
Collaborator

@flowerthrower flowerthrower left a comment

Choose a reason for hiding this comment

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

Thank you very much, @LegionAtol, for incorporating the feedback.
There are a few things left to do but I am sure you will be able to address them easily.
Please ping me, once you are done.

src/qutip_qoc/pulse_optim.py Outdated Show resolved Hide resolved
@@ -84,6 +84,11 @@ def optimize_pulses(
Global steps default to 0 (no global optimization).
Can be overridden by specifying in minimizer_kwargs.

- shorter_pulses : bool, optional
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is basically what the __time__ control parameter is used for.
Of course, RL doesn't really take a 'guess' (as with the other parameters), but we could use the same structure to only look for shorter pulses if the __time__ bounds are specified.

src/qutip_qoc/_rl.py Outdated Show resolved Hide resolved
src/qutip_qoc/_rl.py Outdated Show resolved Hide resolved
src/qutip_qoc/_rl.py Outdated Show resolved Hide resolved
src/qutip_qoc/_rl.py Outdated Show resolved Hide resolved
… added underscore for internal variables, args is now passed as a parameter to _infid(), changes in callback function
…m continues to search for solutions with shorter pulses
@coveralls
Copy link

coveralls commented Sep 30, 2024

Pull Request Test Coverage Report for Build 11141454500

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 162 of 173 (93.64%) changed or added relevant lines in 2 files are covered.
  • 5 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+1.2%) to 88.634%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/qutip_qoc/_rl.py 157 168 93.45%
Files with Coverage Reduction New Missed Lines %
src/qutip_qoc/_optimizer.py 1 85.07%
src/qutip_qoc/_goat.py 1 98.26%
src/qutip_qoc/pulse_optim.py 3 93.68%
Totals Coverage Status
Change from base Build 9845330822: 1.2%
Covered Lines: 811
Relevant Lines: 915

💛 - Coveralls

@flowerthrower
Copy link
Collaborator

@LegionAtol please take one last look at the minor changes I have made.

  1. I reverted to your original solution of using the shorter_pulses kwrd instead of an empty __time__ dict.
  2. I ran pre-commit run -a for proper formatting

Once you sign of on these changes, the PR is ready to merge.

@LegionAtol
Copy link
Contributor Author

Ok, that's fine
thanks Patrick

@flowerthrower flowerthrower merged commit d3ce1de into qutip:main Oct 4, 2024
24 checks passed
flowerthrower added a commit that referenced this pull request Oct 4, 2024
@LegionAtols GSOC RL algorithm
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.

3 participants