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

Migrating Marek's code for DBI to discontinue the old repository #1143

Merged
merged 55 commits into from
Feb 25, 2024

Conversation

Sam-XiaoyueLi
Copy link
Contributor

@Sam-XiaoyueLi Sam-XiaoyueLi commented Dec 20, 2023

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

Hi guys, this is our first pull request. @marekgluza and I have reviewed the code together and to the best of our knowledge it is ready for your review. The changes include:

  1. New file: additional_double_bracket_functions.py includes additional functions, among which the most important ones are select_best_dbr_generator and select_best_dbr_generator_and_run. You may see how it works in the example notebook "DBI_strategies_Pauli-Z_products.ipynb".
  2. To ensure the functions running, it was necessary to make some small changes to the "double_bracket.py" file, you may verify that the changes do not affect the normal routine as in "dbi.ipynb" (now renamed "dbi_tutotrial_basic_intro.ipynb"

In summary, you can try running the "DBI_strategies_Pauli-Z_products.ipynb". This will show you which functions are being used.

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (f9de59c) to head (f84ca33).

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1143   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           70        71    +1     
  Lines        10381     10437   +56     
=========================================
+ Hits         10381     10437   +56     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@andrea-pasquale andrea-pasquale left a comment

Choose a reason for hiding this comment

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

Thanks @Sam-XiaoyueLi for adding these interesting features.
You can find below some comments. Let me know if you need help with any of the points that I raised.

src/qibo/models/dbi/double_bracket.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/additional_double_bracket_functions.py Outdated Show resolved Hide resolved
examples/dbi/DBI_strategy_Pauli-Z_products.ipynb Outdated Show resolved Hide resolved
examples/dbi/dbi_tutorial_basic_intro.ipynb Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
src/qibo/models/dbi/additional_double_bracket_functions.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/additional_double_bracket_functions.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/utils.py Show resolved Hide resolved
src/qibo/models/dbi/utils.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/utils.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/utils.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/utils.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/utils.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/utils.py Show resolved Hide resolved
src/qibo/models/dbi/utils.py Outdated Show resolved Hide resolved
@marekgluza
Copy link
Contributor

Thanks @Sam-XiaoyueLi for adding these interesting features. You can find below some comments. Let me know if you need help with any of the points that I raised.

Andrea, how does it work with merging in this situation? Should we request for your approval or 'somehow' go ahead with merge?

@andrea-pasquale
Copy link
Contributor

Thanks @Sam-XiaoyueLi for adding these interesting features. You can find below some comments. Let me know if you need help with any of the points that I raised.

Andrea, how does it work with merging in this situation? Should we request for your approval or 'somehow' go ahead with merge?

In order to merge the pull request two approvals are needed. I need to check the last changes and then I can approve.

Copy link
Contributor

@andrea-pasquale andrea-pasquale left a comment

Choose a reason for hiding this comment

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

Thanks @Sam-XiaoyueLi.
Sorry for the late review, I have a few suggestions down below.
Apart from those we should be ready to merge.

src/qibo/models/dbi/utils.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/utils.py Show resolved Hide resolved
src/qibo/models/dbi/utils.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/utils.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/utils.py Show resolved Hide resolved
src/qibo/models/dbi/utils.py Show resolved Hide resolved
Copy link
Contributor

@MatteoRobbiati MatteoRobbiati left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Sam-XiaoyueLi for this.

Some suggestions follow. Many of them are very small comments related to the docstrings.
Again on docstrings: I think we should add the utils file to the documentation. I can do this once the last fixes are done.

The tests are failing because the variable CS_angle_sgn is not defined. I think you wanted to call cs_angle_sgn. This will be fixed adding one of my following suggestions.

src/qibo/models/dbi/utils.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/double_bracket.py Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add this utils function to the Qibo documentation.
I can do that just before merging.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @MatteoRobbiati thanks that would be helpful! There's Chinese New Year now so we are both off next week but once @Sam-XiaoyueLi is back it would be great to close the PR! :)

Copy link
Contributor

@MatteoRobbiati MatteoRobbiati Feb 6, 2024

Choose a reason for hiding this comment

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

Sure, no problem. Another solution is that I go ahead with the modifications and ask @andrea-pasquale and @Edoardo-Pedicillo for a final review :) So that everything will be merged when you are back.

OFC if @Sam-XiaoyueLi you are ok with this. Otherwise we can simply wait until you are back!

Copy link
Contributor

Choose a reason for hiding this comment

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

that would be fantastic if you could merge after your changes, thanks @MatteoRobbiati

if there are any issues, you can raise separate issues too and assign @Sam-XiaoyueLi and then we can split the requests into smaller merges. Will be easier to close the PRs? Happy CNY! :)

src/qibo/models/dbi/utils.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/utils.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/utils.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/utils.py Outdated Show resolved Hide resolved
src/qibo/models/dbi/utils.py Outdated Show resolved Hide resolved
tests/test_models_dbi_utils.py Show resolved Hide resolved
tests/test_models_dbi_utils.py Outdated Show resolved Hide resolved
@MatteoRobbiati
Copy link
Contributor

I did some modifications.
Only a couple of @andrea-pasquale's comments are missing.
To me this is almost ready to go.



def select_best_dbr_generator(
dbi_object: DoubleBracketIteration,
Copy link
Contributor

Choose a reason for hiding this comment

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

@MatteoRobbiati @Edoardo-Pedicillo if you agree, I believe that here it would make more sense to pass a hamiltonian as argument. The function should then return the correct DBI instance with the corresponding execution parameters d, step and so on...

@Sam-XiaoyueLi Sam-XiaoyueLi added this pull request to the merge queue Feb 25, 2024
Merged via the queue into master with commit 1eac403 Feb 25, 2024
21 checks passed
@marekgluza marekgluza deleted the dbf_migrate branch April 2, 2024 09:03
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.

7 participants