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

Merge krylovsolve #1739

Merged
merged 59 commits into from
Apr 8, 2022
Merged

Merge krylovsolve #1739

merged 59 commits into from
Apr 8, 2022

Conversation

emilianomfortes
Copy link
Contributor

@emilianomfortes emilianomfortes commented Dec 10, 2021

Description
Addition of krylovsolve as another qutip solver. Krylovsolver has been developed via the Unitary-fund grants program.

Related issues or PRs
The original issue discussing the topic: /issues/1668

Changelog
Added krylovsolve as a new solver based on krylov subspace approximation.

@nathanshammah
Copy link
Member

Great to see this come up. Is it ready for review?

@emilianomfortes
Copy link
Contributor Author

emilianomfortes commented Dec 13, 2021

Hello Nathan, in a couple of hours we will perform the final minor docstring improvements, since we reduced the Cognitive Complexity. Tomorrow is finally ready!

@coveralls
Copy link

coveralls commented Dec 13, 2021

Coverage Status

Coverage increased (+0.5%) to 69.485% when pulling b3ff1c4 on emilianomfortes:merge-krylovsolve into 00d0fe4 on qutip:master.

@ruffa
Copy link
Contributor

ruffa commented Dec 14, 2021

Ready for check!!

Copy link
Member

@Ericgig Ericgig 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 for the PR.

Krylov approximation method seems interesting, they look reasonably fast and have low error.

However to add them to qutip we would need some automated tests. We use pytest, you can look into qutip/tests/test_sesolve.py for how to structure the tests.
You should be able to reuse some of the tests with constant Hamiltonian.

Also please use raise instead of assert. assert can be written on one line and seems nice, but it would be better to raise the proper type of error : TypeError, ValueError...

ps. We probably will not be looking at this much more before mid-January.

qutip/krylovsolve.py Outdated Show resolved Hide resolved
qutip/krylovsolve.py Outdated Show resolved Hide resolved
qutip/krylovsolve.py Outdated Show resolved Hide resolved
qutip/krylovsolve.py Outdated Show resolved Hide resolved
qutip/krylovsolve.py Outdated Show resolved Hide resolved
qutip/krylovsolve.py Outdated Show resolved Hide resolved
qutip/krylovsolve.py Outdated Show resolved Hide resolved
qutip/krylovsolve.py Outdated Show resolved Hide resolved
qutip/krylovsolve.py Outdated Show resolved Hide resolved
qutip/krylovsolve.py Outdated Show resolved Hide resolved
@hodgestar
Copy link
Contributor

@emilianomfortes Hello! I would like to include this in the QuTiP 4.7 release. Do you have time to incorporate or respond to @Ericgig's suggestions?

@hodgestar hodgestar added this to the QuTiP 4.7 milestone Feb 9, 2022
@emilianomfortes
Copy link
Contributor Author

emilianomfortes commented Feb 14, 2022

Hello @hodgestar, we missed your last comment, but Nathan just notified us. We will get it all sorted out by wednesday at most. I leave a todolist to keep track of everything that @Ericgig mentioned.

To-do

  • Include some tests like test_sesolve.py --> Included tests with random Hamiltonian, ising transverse field and SHO. Both for states and expectation values. Lacking a testing example for callable e_ops.

  • Why krylov_dim=30? ? --> There was a subtle physical reason, but we conclude its better to leave it as a free input without a predefined variable.

  • tolerance, store_states and store_final_state --> Changed as an Options qutip class, now tolerance is Options.atol property

  • e_ops should also take a callable or list of mixed function and Qobj --> Added support

  • Remove support for np.ndarray --> Removed for qutip cleanliness

  • Assertion errors to specififc errors --> Modified.

  • Check that psi0 is a ket --> Added

  • If both store_states and store_final_state are True there is a small bug --> Fixed the store twice problem

  • Infinite loop at stagnant =0 --> Added a raise ValueError if its <0.

  • all should be defined to filter functions seen by the user --> set to =krylovsolve and lanczos_algorithm

  • Why not call _make_partitions directly instead of having this method --> Removed when deprecating KSolve class

  • tlist is already stored, are those needed? (lines 152-155) --> Removed when deprecating KSolve class

  • The Result has a tag to the solver that created it --> Added

  • If tlist is empty, you can just return the empty results. --> Now it returns an empty Results() instance.

  • Can you use the eigh in qutip.sparse . There is a bug in eigh on mac with openblas. --> Switched to qutip.sparse.eigh

  • What does this optimizer function optimize? Could you add a simple description. --> This functions finds the optimal number of Lanczos algorithm iterations inside Krylov, added as small description

  • _estimate_norm is nerver used. --> Removed

  • Why is tlist here and not in solve? It seems strange that you can reuse one instance for multiple H and psi0, but you cannot change tlist. --> Removed when deprecating KSolve class

  • Why do you need KSolve? --> It was a workaround to the cognitive complexity check of github. But we decided to revert back to a non Class solver, as it adds another type of complexity.

  • Increasing pdx here when it is already controlled by enumerate is strange. The best would be to include the first partition here too. Otherwise using update_progress_bar(pdx + 1) is clearer. --> Changed "pdx" into "idx". Added the first lazy iteration inside the loop, now it is more clear with update_progress_bar(idx).

  • There is no work done between update_progress_bar(0) and update_progress_bar(1). Did you start it? --> Fixed simultaneously by the previous change.

  • Shouldn't this also double the last element? prepare_next_step remove the first and last so the last state is not stored. --> Good catch, it was a bug. Fixed.

  • _happy_breadkown never used... --> Should be fixed, performing extra checks

  • evolved states It should keep the input state dims. --> Checked all situations that came to mind and output dimensions seem to be working properly.

  • Why import mcsolve? --> Removed

Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

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

Better, thank you for adding tests, now they need to pass.

The dims of the Qobj are ignored, while they are not useful in this method, they should be at least checked and the output states' dims should match the input ones.

qutip/tests/test_krylovsolve.py Outdated Show resolved Hide resolved
qutip/tests/test_krylovsolve.py Outdated Show resolved Hide resolved
qutip/tests/test_krylovsolve.py Outdated Show resolved Hide resolved
qutip/tests/test_krylovsolve.py Outdated Show resolved Hide resolved
qutip/tests/test_krylovsolve.py Outdated Show resolved Hide resolved
qutip/krylovsolve.py Outdated Show resolved Hide resolved
qutip/krylovsolve.py Outdated Show resolved Hide resolved
qutip/krylovsolve.py Outdated Show resolved Hide resolved
qutip/krylovsolve.py Outdated Show resolved Hide resolved
qutip/krylovsolve.py Show resolved Hide resolved
@emilianomfortes
Copy link
Contributor Author

emilianomfortes commented Feb 16, 2022

New changelog

test_krylovsolve.py

  • We have qutip.rand_herm for a random hamiltonian. --> Changed
  • Isn't h_sho equivalent to U=qutip.rand_unitary_haar(dim) \ return U* (qutip.num(dim)+0.5)*U.dag() ? --> We can test it with that one.
  • Clean imports on test_krylovsolve; qeye imported twice, run_module_suite never used, etc.
  • Header no longer used. --> Removed.
  • Why is os.environ['QUTIP_GRAPHICS'] = "NO" needed? --> Remained from the original copy from test_sesolve.py
  • Why not simply compare to sesolve ? --> Is easier, so we swap it.
  • Naming this fidelity feels wrong with the 1-.... Also make Qobj and array conversions more efficiently with Naming this fidelity feels wrong with the 1-....
    You don't need to work go back and forth between Qobj and array. Also make things more efficiently with 1 - np.abs(psi_exact.overlap(psi_k))**2. --> Corrected.
  • More efficiently psi0=qutip.rand_ket(dim). --> Corrected.
  • Why compute 3 expectation values if only one is tested?
    Also please use the normal assert instead of numpy's assert_. We are slowly removing them since we migrated from unitest to pytest. --> Now all of them are checked and using asset

krylovsolve.py

  • isinstance should be tested first since if it isn't, psi0.isket will raise an error before reaching it. It should raise a TypeError.
  • Please raise an error instead of using assert, since it can be suppressed.
  • Testing for dims instead of shape would be better. --> Done.
  • Empty progress bars can be ignored.
  • Improve the print at particular_tlist using warnings.warn. Properly describe the warning.
  • Swap illinois algorithm with a root finder from scipy. --> Now using scipy.optimize.root_finder.
  • n_iterations become delta_t when optimizer is called. It does not seems to be an integer so isn't delta_t a better name here? --> Renamed to delta_t.
  • bound_function seems very generic. Same with optimizer. --> Renamed to _lanczos_error_equation_to_optimize_delta_t and _optimize_lanczos_timestep_size.
  • numpy's array have a dot method that act the same a sparse array's one. This is not needed.
  • The dims of the output are not kept at evolved_states = map(Qobj, evolved_states[1:-1] --> Separated into two variables. Now dimensions are also be kept.
  • Why make lanczos_algorithm part of the public interface? --> Mistake; corrected

emilianomfortes and others added 5 commits March 28, 2022 21:43
Co-authored-by: Eric Giguère <eric.giguere@calculquebec.ca>
Co-authored-by: Eric Giguère <eric.giguere@calculquebec.ca>
@emilianomfortes
Copy link
Contributor Author

It's been a while... but I am finally able to continue. I've cleand a lot, theres probably minor typing fixes in the tests, but I got rid of the general check. Now tests are mostly separated and, although a bit extensive, most things should be covered.

@nathanshammah
Copy link
Member

Looking forward to this @emilianomfortes!

Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

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

Look good,
The test coverage is good.
It's mostly clear what each test does, but some tests share the same docstring.

Parametrization over e_ops could remove some code duplication.

qutip/tests/test_krylovsolve.py Show resolved Hide resolved
@emilianomfortes
Copy link
Contributor Author

@Ericgig

  • docstrings should be fixed

  • coveraged increased with

e_ops = [callable, qobj] e_ops = [callable, callable] e_ops = [qobj, qobj]

  • added a more efficient and clear pytest parametrization
  • fixed the repeated sparse test

Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

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

What I wanted to say is that test 7 and 8, etc. could be merge together by parametrizing over e_ops. One test can use multiple @pytest.mark.parametrize to go over a grid of possibilities.

@emilianomfortes
Copy link
Contributor Author

Thanks Eric, I've learned so much from your comments.

@emilianomfortes
Copy link
Contributor Author

A check on macOS wasn’t successful, but I don’t see how that error message could have any relation with krylovsolve

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 added krylovsolve to the API docs and fixed some documentation and code formatting issues.

@hodgestar hodgestar merged commit 1b1f6df into qutip:master Apr 8, 2022
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.

None yet

6 participants