oimeitei / quemb

QuEmb is an open-source tool for efficient quantum chemistry simulation of large molecules and solids (1D and 2D periodic systems) via bootstrap embedding technique.
Apache License 2.0
4 stars 6 forks source link

BE-DMRG Functionality and More #40

Closed ShaunWeatherly closed 1 month ago

ShaunWeatherly commented 1 month ago

BE-DMRG

QOL Improvements

Attributions

mscho527 commented 1 month ago

We've trying to keep the PRs bite-sized. Could you split this so that each PR adds/fixes one thing at a time? Thanks! https://bssw.io/blog_posts/pull-request-size-matters

ShaunWeatherly commented 1 month ago

As per your blog post:

Always maximizing productivity for reviewers and the review process may create unwelcome productivity issues for other developers and collaborators submitting PRs. So far, we've focused on the productivity of the PR review process and what submitters can do to minimize review effort. However, there are productivity costs for submitters too.

First, strict adherence to avoiding mixing unrelated changes in a single PR may simply discourage some good housekeeping (while I am here fixing problem A, why don't I go ahead and also fix problem B). There is just a lot of overhead associated with splitting changes over multiple PRs including creating branches and PRs, running and waiting for CI in each PR, ensuring each PR is reviewed and merged in logical order and possibly having to apply each PR's changes to multiple branches (e.g. release candidate and main).

Also, the many small fixes will be single line PRs, and I don't think that's a valuable use of anyone's time.

mscho527 commented 1 month ago

Your PR contains several separable parts that do not depend on each other. Separating the PR into at least three (but not 12 lol) --

would be advantageous, especially because some of your commits are better squash-merged for clarity in the git history. The overhead you mention doesn't quite apply here, because the PRs would not be interdependent. (therefore does not require rebasing) Obviously, I can't oblige anyone to follow this, but such consideration facilitates collaborative code development.

mscho527 commented 1 month ago

pytest fails for the added DMRG test. Please check and modify the code as necessary.

ShaunWeatherly commented 1 month ago

This is an import error, which has reminded me that the dmrgscf module is not a standard import for pyscf. As unit tests shouldn't depend on optional imports, I'm thinking it's probably best to remove this test entirely.

ShaunWeatherly commented 1 month ago

Your PR contains several separable parts that do not depend on each other. Separating the PR into at least three (but not 12 lol) --

  • BE-DMRG related commits
  • Docs improvement
  • Pipek Mezey Feat

would be advantageous, especially because some of your commits are better squash-merged for clarity in the git history. The overhead you mention doesn't quite apply here, because the PRs would not be interdependent. (therefore does not require rebasing) Obviously, I can't oblige anyone to follow this, but such consideration facilitates collaborative code development.

I'll apologize but maintain that these changes are related, other than the docs which would comprise very minor PRs.

Moreover, the purpose of splitting PRs is to distribute work among different reviewers, but as you're currently the only one of us with permission to merge I fail to see how the extra work on my part will will aid in this task.

Finally, I think whatever standards we agree to should be agreed upon as a group, as this is a codebase that the group will build on, and this requires at the very least an open discussion of these topics rather than one person making these decisions seemingly behind closed doors. Respectfully, I think such considerations would actually facilitate collaborative code development.

mscho527 commented 1 month ago

Just so you know, I am not the only one with the permission to merge. Bite-sizing the PRs was agreed amongst the people who contributed in the past months. I am applying the same standards here.

oimeitei commented 1 month ago

Bite-sizing PRs are not only to ease review tasks but maintains better version control and help in isolating issues later on. This is more or less a standard practice everywhere. I am closing this PRs for now.