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

Added modules for some common operator pools, lattice Hamiltonians, and other ADAPT variants #1

Merged
merged 16 commits into from
May 15, 2024

Conversation

KarunyaShirali
Copy link
Contributor

@KarunyaShirali KarunyaShirali commented May 9, 2024

Added

  • a suite of common operator pools
  • a spin-chain lattice Hamiltonian
  • a variant of ADAPT (RandomADAPT) in which the operator to grow the ansatz with is chosen randomly when the scores are degenerate

@kmsherbertvt
Copy link
Owner

I realized the pull request just shows up as a regular Github issue and I can type comments write here. ^_^

I presently have three simple requests, and two somewhat more involved:

  1. I see you have included LatticeModelHamiltonians and OperatorPools within the "Basics" module, similar to how I had Operators. Well, I don't actually like Operators. :P Personally I think LatticeModelHamiltonians should be a separate module, done similar to Random_ADAPT. Also, your changes will just completely replace the Operators module, so it'd be helpful if you can just copy/paste the hubbard implementation into your new module.
  2. After reflection, I do think I like all the OperatorPools you've implemented to indeed be considered "Basics" (possibly I'd prefer the tiling function to be in its own separate module but I don't have a super strong feeling about that, so I'll leave it to your judgement). I think we can simplify the name to just Pools? Finally, since it is a basic component, you can move the pools.jl file into basics, and get rid of the pools folder.
  3. Can you move the tiling_XXZ.jl script into the test folder, so it is side by side with its sibling hubbard_qeb.jl? Technically test is not the best place for them, but I am content to put proof-of-concept/tutorial scripts there for now.

@kmsherbertvt
Copy link
Owner

Speaking of tests, I've remembered there are two more things to worry about when adding code to ADAPT.jl: tests, and docs.

Tests

In the test/runtests.jl file, you should add a "@testset" for each module you've added, or at least for Random_ADAPT. I don't necessarily think we need to worry about all the hamiltonians and pools.

I've already got a nice infrastructure set up to check for internal consistency; you can probably model your testset after the one for Overlap and OptimizationFree. Although it sure looks to me like there's a typo in the OptimizationFree test, and I'm a bit puzzled how that hasn't caused problems...

Anyway, right before you make any pushes, it's a good practice to run the tests, by starting up a fresh Julia REPL and including the runtests.jl script. To be doubly sure, you can run ] test, which takes an annoyingly long time but it's what the continuous integration actually does to run the tests so it can catch stupid problems that might have come up with the project and manifest files.

@kmsherbertvt
Copy link
Owner

Docs

Inside of docs/src/index.md, you've got to add any new modules. So under the "Basics" header you'd add OperatorsPools (or just Pools if you rename it), and under the "Other Modules" header you'd add the other two.

I think that's all you should have to do there...

@kmsherbertvt
Copy link
Owner

Also, super sorry to be nit-picky but I just realized a few more minor items:

  1. Please remove all the .DS_Store files from the git project. These are incredibly frustrating files automatically generated by the OS operating system to help it do something stupid and irrelevant. They should never be committed to a git project. (You can make this easier on yourself by adding ".DS_Store" to your .gitignore file, so that doing something like git add * won't add them.)

  2. I do not believe that .../.julia/environments/v1.9.3 should be in the git project either. I'm not sure if you did that intentionally? Maybe you have some kind of symbolic link in your local project?

  3. The Manifest.toml file is also something of a problem. I'm not sure how we're supposed to handle it. I think you need it, in your forked repository, for the continuous integration to be able to find the PauliOperators.jl package. But it seems that your project hash differs from mine, and that's...probably not good.

    Also, did you add the Combinatorics package via a github url? If you needed to do that, that may also complicate things, since I think I would need that part of your manifest. But I'm not sure you need to have done that... Perhaps we'd best discuss this part in person... o_O

…l script to test folder

Created a separate Hamiltonians module, included everything that was in Operators in `Pools` (previously, OperatorPools), moved tiling_XXZ.jl into the test folder, removed the Combinatorics dependency (was unnecessary), removed .DS_store
Moved Hubbard definition into latticemodels.jl from pools.jl, added the documentation to docs
Renamed randomADAPT and its directories and files to the more appropriate `degenerateADAPT`
Added three functions: Base.:≈ and two otimes functions that should ultimately be added to the PauliOperators.jl package
Updated the Hubbard QEB tutorial script to use the Hamiltonian and pool defined in ADAPT.Hamiltonians and ADAPT.Pools respectively
Removed changes to .gitignore
Added a testset for the module Degenerate_ADAPT, test successful
Need to fix qubit ADAPT pool and decide location for tile_operators function
@kmsherbertvt kmsherbertvt merged commit e968715 into kmsherbertvt:main May 15, 2024
2 of 4 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.

2 participants