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

Get FC import from CASTEP 25.1 working #364

Merged
merged 5 commits into from
Feb 10, 2025
Merged

Conversation

ajjackson
Copy link
Collaborator

@ajjackson ajjackson commented Feb 5, 2025

Closes #294

  • CASTEP 25.1 adds extra fields to BORN and DIELECTRIC sections of castep_bin file; read (and discard) this information
  • It is also now possible to create a BORN section without corresponding DIELECTRIC, using a finite-displacement scheme with ultrasoft pseudopotentials. That throws off some Euphonic logic which assumes you would only ever have BORN and DIELECTRIC data at the same time.
  • As FC behaviour isn't really defined when you only have one, let's discard it in that case.

I need to test a wider range of import/present/not-present cases to be sure this will always do the right thing.

The good news is that most cases seem to work without this update; the main issue is with Berry-phase Born charge calculations.

@ajjackson ajjackson added the bug Something isn't working label Feb 5, 2025
@ajjackson ajjackson added this to the v1.4.1 milestone Feb 5, 2025
Copy link
Contributor

github-actions bot commented Feb 5, 2025

Test Results

   21 files  +  3     21 suites  +3   1h 3m 43s ⏱️ + 5m 0s
1 083 tests +  2  1 077 ✅ +  2   6 💤 ±0  0 ❌ ±0 
9 709 runs   - 765  9 651 ✅  - 765  58 💤 ±0  0 ❌ ±0 

Results for commit 7f7bab3. ± Comparison against base commit 1d9874a.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.04%. Comparing base (48f8d11) to head (799b1d1).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #364   +/-   ##
=======================================
  Coverage   96.03%   96.04%           
=======================================
  Files          31       31           
  Lines        4214     4219    +5     
  Branches      643      645    +2     
=======================================
+ Hits         4047     4052    +5     
  Misses         95       95           
  Partials       72       72           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ajjackson ajjackson force-pushed the 294-castep-bin-format branch from 16683cd to d0b6851 Compare February 6, 2025 15:41
@ajjackson
Copy link
Collaborator Author

Note that in the new test reference data:

  • For a CASTEP 25 calculation which loads from an external BORN file, the Born effective charges and dielectric tensor are included in the JSON as "usual"
  • For a CASTEP 25 calculation which calculations Born effective charges by finite displacements and does not have a corresponding dielectric tensor, the Born effective charges are omitted from the Force Constants. (They cannot be used to correct frequencies and no NAC term has been subtracted from the Force Constants, so it would be misleading to include them.)

@ajjackson
Copy link
Collaborator Author

I deliberately pushed a failing commit with updated tests, to verify that the new tests cover a real problem. The actual fix is in the following commit.

- CASTEP 25.1 adds extra fields to BORN and DIELECTRIC sections of
  castep_bin file; read (and discard) this information

- It can also create a BORN section without corresponding
  DIELECTRIC (by Berry Phase calculation). That throws off some Euphonic
  logic which assumes you would only ever have BORN and DIELECTRIC data
  at the same time.

- As FC behaviour isn't really defined when you only have one, let's
  discard it in that case.
@ajjackson ajjackson force-pushed the 294-castep-bin-format branch from 254b47c to 9a35420 Compare February 6, 2025 15:53
@ajjackson ajjackson marked this pull request as ready for review February 6, 2025 15:53
Copy link
Collaborator Author

@ajjackson ajjackson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I got slightly carried away with whitespace

euphonic/readers/castep.py Outdated Show resolved Hide resolved
@ajjackson ajjackson requested a review from oerc0122 February 9, 2025 21:49
Copy link
Collaborator

@oerc0122 oerc0122 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible and simple. Concerns about changing the process generally when it should presumably only affect 25.1 files?

euphonic/force_constants.py Show resolved Hide resolved
euphonic/readers/castep.py Show resolved Hide resolved
@ajjackson ajjackson mentioned this pull request Feb 9, 2025
@ajjackson
Copy link
Collaborator Author

Forgot the changelog 😅

@ajjackson ajjackson requested a review from oerc0122 February 10, 2025 16:24
Copy link
Collaborator

@oerc0122 oerc0122 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here I thought FC stood for Forget the Changelog.

Looks good!

@ajjackson ajjackson enabled auto-merge (squash) February 10, 2025 17:07
@ajjackson ajjackson merged commit 0554c1f into master Feb 10, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forthcoming (release 25.1) changes to CASTEP checkpoint format require Euphonic update
2 participants