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

Fixing Pyscf Incompatibility problem #394

Merged

Conversation

anushkrishnav
Copy link
Contributor

@anushkrishnav anushkrishnav commented May 31, 2024

Fixes #383

The problem inherently lies with PYSCF which they have corrected a few weeks ago, for the time being, my fixes will help remove the issues with the tests, this should be future-proof since it checks if there is any changed in mf_coeff so even if later psf version rolls out with the fix , the code would still work as there must be a change in mf_coeff which will then not trigger the if condition

had a lot of fun working on this issue, initially though it was something to do with the data structure of mo_coeff then saw it was a pyscf issue

@alexfleury-sb or @ValentinS4t1qbit Hopefully this would help close the issue for now

@ValentinS4t1qbit
Copy link
Collaborator

ValentinS4t1qbit commented May 31, 2024

Maybe @alexfleury-sb has suggestions on how to handle the fix, thanks to your investigation. Alexandre, the automated tests currently force PySCF 2.4.0: do we have a clever way to enable @anushkrishnav to run the test suite on 2.5.0 in his PR ?

@anushkrishnav Before giving you a review, I think you should look at the "checks" at the bottom of this page, click "details" and see what tests are failing and why. Right now, the failures suggest that things are not working for whatever version of pyscf is used in the automated tests (see above). You have an easy way to check the tests locally on your machine: check out the README to see how to run tests locally with unittest, and swap versions of PySCF to see if it works for both.

It is btw perfectly acceptable to have a conditional statement that literally tests the version of pyscf at runtime and does 2 different things depending on the version detected (e.g before and after 2.5.0?). I'd rather be clear and explicit about why things are done, otherwise we'll end up with unexpected instructions for which we do not really remember the purpose.

@alexfleury-sb
Copy link
Collaborator

I am fairly positive that changing https://github.com/goodchemistryco/Tangelo/blob/6791dc83d9dac7ee5aad9d5a1f3dcbed81aac2cb/.github/workflows/continuous_integration.yml#L76 would indeed change the pyscf in the CI for this specific PR. We could change it to something like pyscf>=2.5.0 to make sure it is not going back to 2.4.0 for whatever reasons I can't think of at the moment (unless there are mechanisms in the code to handle the pyscf versions at runtime).

@anushkrishnav
Copy link
Contributor Author

anushkrishnav commented May 31, 2024

The investigation took the most time tbh,

It is btw perfectly acceptable to have a conditional statement that literally tests the version of pyscf at runtime and does 2 different things depending on the version detected (e.g before and after 2.5.0?). I'd rather be clear and explicit about why things are done, otherwise we'll end up with unexpected instructions for which we do not really remember the purpose. (I can add a comment on that to make things clear )

@ValentinS4t1qbit
We don't really need a before and after, the whole thing was caused by a bug from pyscf side, In the issue I have detailed what going on, and why this error happened in the first place. , This issue has now been fixed from pyscf side, I guess the next version will have that fix, this issue that we had was not a feature rather a bug from their side

pyscf/pyscf@9a152a9

@anushkrishnav
Copy link
Contributor Author

anushkrishnav commented May 31, 2024

So my next step will be the testcases rt ? I can try and toggle between version locally, I was using 2 machines to trace the error using the 2 version, Is there a better way to do it ? I think rt now the issue is we are not yet able to run tests on 2.5 rt ?

@ValentinS4t1qbit
Copy link
Collaborator

@anushkrishnav Thank you for investigating this ! Sorry for the delay, weekend + international travels.

Local testing : I think an easy way to test if your code works for both is simply to test in your local environment one version, then uninstall pyscf with pip uninstall -y pyscf and then install the other version with pip install pyscf==x.y.z. We do not know what versions of pyscf our users rely on and do not know when the new pyscf version with the fix will be released: i'd rather we have a conditional statement for now (if pyscf.__version__ < 2.5.0 or something like that).

Automated tests on Github: In order for automated tests to run with the desired version of pyscf, you can modify the yml workflow file in .github/workflows/continuous_integration.yml. If you scroll down, you'll see that @alexfleury-sb enforced pip install pyscf==2.4.0 after we saw the latest version was troublesome. Here I suggest you replace it by pip install pyscf to get the latest (this way when the fix is released, it will be used).

Your goal is for these tests to pass !

@anushkrishnav
Copy link
Contributor Author

@anushkrishnav Thank you for investigating this ! Sorry for the delay, weekend + international travels.

Local testing : I think an easy way to test if your code works for both is simply to test in your local environment one version, then uninstall pyscf with pip uninstall -y pyscf and then install the other version with pip install pyscf==x.y.z. We do not know what versions of pyscf our users rely on and do not know when the new pyscf version with the fix will be released: i'd rather we have a conditional statement for now (if pyscf.__version__ < 2.5.0 or something like that).

Automated tests on Github: In order for automated tests to run with the desired version of pyscf, you can modify the yml workflow file in .github/workflows/continuous_integration.yml. If you scroll down, you'll see that @alexfleury-sb enforced pip install pyscf==2.4.0 after we saw the latest version was troublesome. Here I suggest you replace it by pip install pyscf to get the latest (this way when the fix is released, it will be used).

Your goal is for these tests to pass !

I have already tested mine on local, I installed and uninstalled the package to see how it performs. I will now move to the automated tests, is it something that I can do ? updating the workflow?

@ValentinS4t1qbit
Copy link
Collaborator

I have already tested mine on local, I installed and uninstalled the package to see how it performs. I will now move to the automated tests, is it something that I can do ? updating the workflow?

Yes, that is what the 2nd paragraph of my previous answer was suggesting

@anushkrishnav
Copy link
Contributor Author

Updated the CI file

@anushkrishnav
Copy link
Contributor Author

anushkrishnav commented Jun 10, 2024

Not sure how do I navigate the issue, there has not been any change except the one made to get it back to older state , when on newer version, this rectifies the mistake made on pyscf side. I am not sure about the final test, it is supposed to be skipped on pyscf rt ? @ValentinS4t1qbit

Copy link
Collaborator

@ValentinS4t1qbit ValentinS4t1qbit 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 so much for working on this !

  • you can definitely ignore the MINDO3 test currently failing, this has nothing to do with your change, we'll create an issue for it :)
  • I don't quite understand some of your changes. @alexfleury-sb would you mind having a look to help him take this to the finish line ?

tangelo/algorithms/classical/mp2_solver.py Show resolved Hide resolved
tangelo/algorithms/classical/mp2_solver.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@alexfleury-sb alexfleury-sb left a comment

Choose a reason for hiding this comment

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

@ValentinS4t1qbit This is a good solution to our previous problems. As I read in the thread, the bug was fixed on pyscf side and is only appearing with version 2.5.0.

For the MINDO tests that fail, I can take care of them! I think this is out of scope for this PR.

@anushkrishnav
Copy link
Contributor Author

@ValentinS4t1qbit This is a good solution to our previous problems. As I read in the thread, the bug was fixed on pyscf side and is only appearing with version 2.5.0.

For the MINDO tests that fail, I can take care of them! I think this is out of scope for this PR.

Awesome , So hopefully this is a wrap for the problem ?

@anushkrishnav
Copy link
Contributor Author

Thank you for the help and guidance @ValentinS4t1qbit

Copy link
Collaborator

@ValentinS4t1qbit ValentinS4t1qbit 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 so much for the hard work @anushkrishnav !

If you have any suggestions or feedback after playing a bit with this 🍊 Tangelo 🍊 , please don't hesitate ! Otherwise, just leaving us a ⭐ on Github if you think this project is interesting to support us :)

@ValentinS4t1qbit ValentinS4t1qbit merged commit 217d975 into sandbox-quantum:develop Jun 12, 2024
5 of 9 checks passed
ValentinS4t1qbit added a commit that referenced this pull request Jun 18, 2024
#397)

* Improving draw method: now works with strings for gate parameters (#391)
* Added support for multi-controlled gates on qiskit (MCRX, MCRY, MCRZ, MCPHASE) (#390)
* Add automated documentation workflow & gitignore (#396)
* Unitaryhack issue 386: Augment the Rotosolve optimizer to support Rotoselect (#392)
* ⚡Added rotoselect algorithm and improved rotosolve to use 2 evaluations per parameter.
* Unitaryhack issue 384: Circuit as reference state in the ansatz definition (#398)
* added method `Circuit.fix_variational_parametrs`, which turns all variational gates non-variational.
* Added support for circuits referrence states for the ansatze
* Fixing Pyscf Incompatibility problem  (#394)
* update CI to check for version 2.5
* Update mp2_solver.py

---------

Co-authored-by: AlexandreF-1qbit <[email protected]>
Co-authored-by: ValentinS4t1qbit <[email protected]>
Co-authored-by: Alexandre Fleury <[email protected]>
Co-authored-by: Valentin Senicourt <[email protected]>
Co-authored-by: GitHub Actions <[email protected]>
Co-authored-by: Anush Venkatakrishnan <[email protected]>
Co-authored-by: Or Golan <[email protected]>
Co-authored-by: Kazuki Tsuoka <[email protected]>
Co-authored-by: Colin Burdine <[email protected]>
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