-
Notifications
You must be signed in to change notification settings - Fork 8
Azam/feature/hardcoded biorthogonal coefficients #457
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
base: master
Are you sure you want to change the base?
Azam/feature/hardcoded biorthogonal coefficients #457
Conversation
…nal_coefficients' into azam/feature/hardcoded_biorthogonal_coefficients
…eature/hardcoded_biorthogonal_coefficients # Conflicts: # SeQuant/domain/mbpt/biorthogonalization.cpp
…eature/hardcoded_biorthogonal_coefficients
…eature/hardcoded_biorthogonal_coefficients
…se use computed ones
evaleev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some initial changes. Use these to go over the PR and address possible other places where these comments apply.
| bool using_hardcoded = false; | ||
|
|
||
| if (n_particles <= 6) { | ||
| std::cout << "using hardcoded biorthogonalization coefficients for rank = " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed, and the same below.
| if (range_rank <= 1) return input_arr; | ||
| btas::Tensor<Args...> result{input_arr.range()}; | ||
| result.fill(0); | ||
| const auto lannot = perm; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? You can directly use perm in the btas::permute call, right?
| return eval_result<this_type>( | ||
| biorthogonal_nns_project_ta(get<ArrayT>(), bra_rank)); | ||
| if constexpr (std::is_integral_v<numeric_type>) { | ||
| std::abort(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switch to SEQUANT_ABORT and with an error message.
| return eval_result<ResultTensorBTAS<T>>( | ||
| biorthogonal_nns_project_btas(get<T>(), bra_rank)); | ||
| if constexpr (std::is_integral_v<numeric_type>) { | ||
| std::abort(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
| #include <vector> | ||
|
|
||
| namespace sequant { | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functions in this file need to be documented. Since the biorthogonalization logic is in the mbpt namespace, makes sense to move these components there, or even to a detail namespace.
PS: In case you are moving these to mbpt::detail namespace, merge with the latest master to avoid namespace ambiguity issues.
| if (!first_row.empty()) { | ||
| return make_hardcoded_biorth_coeffs_matrix(first_row, n_particles); | ||
| } | ||
| return Eigen::Matrix<sequant::rational, Eigen::Dynamic, Eigen::Dynamic>(0, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why return an empty matrix?
|
|
||
| target_link_libraries(SeQuant PUBLIC range-v3::range-v3 Boost::regex Boost::locale Boost::headers SeQuant-bliss Threads::Threads) | ||
| target_link_libraries(SeQuant PRIVATE libperm::libperm $<BUILD_LOCAL_INTERFACE:utfcpp>) | ||
| target_link_libraries(SeQuant PUBLIC libperm::libperm $<BUILD_LOCAL_INTERFACE:utfcpp>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, I would suggest/prefer keeping these dependencies private.
This branch provides hardcoded biorthgonal and nns coefficients