8000 Final Geometry & Periodicity before merge to main by mcsloy · Pull Request #88 · tbmalt/tbmalt · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Final Geometry & Periodicity before merge to main #88

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: development
Choose a base branch
from
Open

Final Geometry & Periodicity before merge to main #88

wants to merge 4 commits into from
+344 −315

Conversation

mcsloy
Copy link
Contributor
@mcsloy mcsloy commented Jun 3, 2025

This commit contains the final clean up pass of the Geometry and Periodicity classes before the merge to main.

@mcsloy mcsloy force-pushed the GeometryCleanup branch from 75a96a6 to 5794d0e Compare June 3, 2025 12:18
@mcsloy
Copy link
Contributor Author
mcsloy commented Jun 3, 2025 •
edited
Loading

The only outstanding questions are:

  1. Could we get away with swapping the floor operation within Periodicity.get_cell_translation_vector_indices for ceiling and then removing the positive_extension and negative_extension components all together? Done.
  2. Why does the coulomb module explicitly set the positive and negative extensions to zero? No longe relevant.
  3. Would it not be better to just have a single unified cutoff distance value rather than having to carry around a "base cutoff" value along with a "tail distance" value only to always combine them together? The code should only cares about how far it should look to find neighbors, and as such should neither know nor care about where the final value comes from. Just having a single "cutoff" value would help improve clarity and consistency when the cutoff is reintroduced to the Geometry class to help cull distance searches. Done

WbSun723
WbSun723 reviewed Jun 12, 2025
View reviewed changes
Simplified cutoff related functionality
2259886 
The `cutoff` and `tail_distance` values have been unified so that only a single distance value, `cutoff`, needs to be provided to a `Geometry` instance.

The `positive_extension` and `negative_extension` values have been removed as they are no longer necessary due to changes in the `Periodicity.get_cell_translation_vector_indices` method.

The `Periodicity` class no longer permits units other than Bohr, and the `_check` method may no longer modify attributes of the `Geometry` instance that is creating it.

Documentation of the `Periodicity` class has been updated to indicate that instances should not be created manually. Rather this should be left to the `Geometry` class. At least until the recursion issue can be resolved.
@mcsloy
Copy link
Contributor Author
mcsloy commented Jun 12, 2025 •
edited
Loading

@WbSun723 I have made the main changes to the Geometry and Periodicity classes. It is at the stage where you can do a general review.

However, I will need to identify why the unit tests are failing. The failure seems to be sporadic, even when the PR merge is pulled and run locally. If I run pytest repeatedly it will error out in the test_dftb_periodic.py unit test, but not always and not in the same function. If I run only the test_dftb_periodic.py unit then I never see the error. This sort of thing is never good news, I will dig about a little and see what I can find.

From what I can tell this is a combination of three things:

  1. There is an abnormally high error associated with the test system SiC_cubic_pbc as used within test_dftb_periodic.py. This is true even when the system is simulated on its own. We may want to regenerate the data to double check.
  2. This is exacerbated by some change in behavior or noise that occurs when operating on a batch of systems. I do not know if this is due to a genuine error caused by the batched operations or a consequence of mixing different properties. For example when batching systems the number of cells present for the smaller system might increase. Looks like the delta between batched and non-batched tests, and thus the higher likelihood of failure, results from absent mixer tolerance values.
  3. We do not set a relative tolerance value in the torch.allclose function which might cause flaky behavior. I have added a relative tolerance value, however it has been set much looser than I would like, until we can resolve issue 1.

Fixed: missing tolerances in test_dftb_periodic
34f5846 
Added missing mixer tolerance parameters to batched periodic SCC DFTB unit test within `test_dftb_periodic.py` which would cause sporadic unit test failures.

Note that the `SiC_cubic_pbc` system still has grater than anticipated errors. The test data for this system should be regenerated to verify its integrity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WbSun723 WbSun723 WbSun723 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mcsloy @WbSun723
0