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

Add mps_add function #1

Closed
wants to merge 17 commits into from

Conversation

MatthiasReumann
Copy link
Contributor

Changelog

  • Add mps_add function (More or less a 1:1 copy of the PyTeNet implementation)
  • Add test_mps_add test function (Generate Random MPS ➡ mps_add ➡ Compare state-vectors)
  • Update README.md (Add dependency installation instructions for MacOS)
  • Add .vscode/ to .gitignore

@cmendl
Copy link
Collaborator

cmendl commented Oct 16, 2024

Thanks for the nice work! Could you modify the pull request by additionally taking these suggestions into account?

  • The data type of the logical output MPS should also be struct mps* (instead of a void pointer). Use allocate_empty_mps to allocate it inside mps_add.
  • Try to rebase the new branch to the latest version of the main branch.
  • For consistency, please start comments with lower-case and without a final dot.
  • In the test, use a literal number instead of "site_array_len" (since "int site_array[site_array_len]" is not allowed in standard C).
  • The test function "char* test_mps_add()" should come directly after char* test_mps_vdot() (to follow the same order as in mps.c)

@MatthiasReumann
Copy link
Contributor Author

Thanks for the nice work! Could you modify the pull request by additionally taking these suggestions into account?

  • The data type of the logical output MPS should also be struct mps* (instead of a void pointer). Use allocate_empty_mps to allocate it inside mps_add.
  • Try to rebase the new branch to the latest version of the main branch.
  • For consistency, please start comments with lower-case and without a final dot.
  • In the test, use a literal number instead of "site_array_len" (since "int site_array[site_array_len]" is not allowed in standard C).
  • The test function "char* test_mps_add()" should come directly after char* test_mps_vdot() (to follow the same order as in mps.c)

Thanks for the feedback! I've updated the code accordingly.

@cmendl
Copy link
Collaborator

cmendl commented Oct 18, 2024

Thanks again! Minor additional point: the local variable qsite is allocated but not freed; actually, instead of allocating this array, one can directly pass psi->qsite to allocate_empty_mps (since it creates its own copy internally).

@MatthiasReumann
Copy link
Contributor Author

Thanks again! Minor additional point: the local variable qsite is allocated but not freed; actually, instead of allocating this array, one can directly pass psi->qsite to allocate_empty_mps (since it creates its own copy internally).

I've removed the qsite copying now, thanks!

@cmendl
Copy link
Collaborator

cmendl commented Oct 18, 2024

Final minor point: currently the automatic unit test does not run due to const qnumber qsite_chi[d] in the unit test; please use an explicit integer here as well, i.e., const qnumber qsite_chi[2]

@MatthiasReumann
Copy link
Contributor Author

Final minor point: currently the automatic unit test does not run due to const qnumber qsite_chi[d] in the unit test; please use an explicit integer here as well, i.e., const qnumber qsite_chi[2]

Ah yes, sorry for that.

@MatthiasReumann
Copy link
Contributor Author

Interesting. Locally ctest works without any troubles. I will build and run the tests on lxhalle (or similar) soon and try to fix the unit test.

@cmendl
Copy link
Collaborator

cmendl commented Oct 18, 2024

There is a problem in the "qnumber_all_equal" assertions: the first argument should be the logical dimension instead of "ndim", for example, assert(qnumber_all_equal(chi_a.dim_logical[0], ...)). Also, in the unit test, the quantum number sector qnum_sector = encode_quantum_number_pair(q_pnum, q_spin) leads to MPS which are logical zero vectors (since the quantum number restriction cannot be satisfied); instead, you can set qnum_sector to a small integer.

@cmendl
Copy link
Collaborator

cmendl commented Oct 19, 2024

Thanks! I've now squash-merged the pull request into the main branch.

@cmendl cmendl closed this Oct 19, 2024
@MatthiasReumann
Copy link
Contributor Author

Great - thanks for the patience and sorry for the hiccups!

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