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 types for core.periodic_table/bonds/composition/ion/lattice/libxcfunc, new type MillerIndex and fix Lattice hash #3814

Merged
merged 38 commits into from
May 12, 2024

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented May 8, 2024

Summary

Add types annotations for:

  • core.periodic_table
  • core.bonds
  • core.composition
  • core.ion
  • core.lattice
  • core.libxcfunc

New custom type:

  • New custom type MillerIndex = tuple[int, int, int]

Bug fixes:

  1. Seem like a typo, I assume it should check if HO exists and replace them with OH. So the condition should be if (self.composition.get("H") and self.composition.get("O")) is not None:, fixed in b4a70ee:
    if self.composition.get("H") == self.composition.get("O") is not None:
    formula = formula.replace("HO", "OH")
  2. Magic number for hash of Lattice fixed in 88e4eda:
    def __hash__(self) -> int:
    return 7

Need confirmation:

  1. In 710cb91, access to Lattice._pbc has been redirected to Lattice.pbc, with a setter method. Wondering if there is any reason for the previous implementation?
  2. Is the following needed by anything?
    if __name__ == "__main__":
    for xc in LibxcFunc:
    print(xc)

Copy link

coderabbitai bot commented May 8, 2024

Note

Reviews Paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Walkthrough

The updates to pymatgen/core/periodic_table.py enhance functionality and consistency in the representation of chemical elements. Key changes include making certain properties non-optional, introducing methods for comparison and representation, and refining type annotations for more precise data handling. These modifications improve the robustness and usability of the module, facilitating easier manipulation and comparison of element properties in scientific computations.

Changes

File Summary of Changes
.../periodic_table.py - Non-optional properties for atomic radius and mass
- Added comparison and representation methods
- Updated type annotations for radii properties
- New method to create elements from names
- Modified validity check for symbols

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@DanielYang59
Copy link
Contributor Author

@coderabbitai ignore

pymatgen/core/periodic_table.py Show resolved Hide resolved
pymatgen/core/periodic_table.py Show resolved Hide resolved
pymatgen/core/periodic_table.py Show resolved Hide resolved
@DanielYang59
Copy link
Contributor Author

DanielYang59 commented May 8, 2024

@janosh Can you please review this at your convenience? Feel free to revert 24b1546 though (I personally don't like to have dunder methods scattered around the class randomly, but such change badly pollute Git Blame). Thanks!

@DanielYang59 DanielYang59 changed the title Tweak core.periodic_table types and docstring Tweak core.periodic_table/composition types and docstring May 9, 2024
@DanielYang59

This comment was marked as outdated.

@DanielYang59 DanielYang59 marked this pull request as draft May 9, 2024 03:45
@DanielYang59 DanielYang59 marked this pull request as ready for review May 9, 2024 03:59
@DanielYang59 DanielYang59 marked this pull request as draft May 9, 2024 12:11
pymatgen/core/ion.py Outdated Show resolved Hide resolved
@DanielYang59 DanielYang59 changed the title Add types for core.periodic_table/bonds/composition/ion/lattice/libxcfunc Add types for core.periodic_table/bonds/composition/ion/lattice/libxcfunc, new type MillerIndex and fix Lattice hash May 12, 2024
@DanielYang59 DanielYang59 marked this pull request as ready for review May 12, 2024 09:59
@DanielYang59
Copy link
Contributor Author

Please review this one as well @janosh. Appreciate your time!

@DanielYang59 DanielYang59 requested a review from JaGeo as a code owner May 12, 2024 12:33
Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

thanks @DanielYang59, lgtm! 👍

@janosh janosh enabled auto-merge (squash) May 12, 2024 12:33
@janosh janosh merged commit 578d29c into materialsproject:master May 12, 2024
23 checks passed
@DanielYang59 DanielYang59 deleted the ptable branch May 12, 2024 12:46
@DanielYang59
Copy link
Contributor Author

Thanks for reviewing @janosh!

Do you have any comments on the "Need Confirmation" section of the PR summary #3814 (comment)?

Regarding (1), I noticed multiple part of core.lattice access self._matrix instead of self.matrix where matrix is a property. I'm not sure if this is intended? Does it offer any benefit that I'm not aware of?

For example (not exhaustive):

def __str__(self) -> str:
return "\n".join(" ".join([f"{i:.6f}" for i in row]) for row in self._matrix)

@property
def metric_tensor(self) -> np.ndarray:
"""The metric tensor of the lattice."""
return np.dot(self._matrix, self._matrix.T)

Is it related to:

Properties lazily generated for efficiency.

@materialsproject materialsproject deleted a comment from coderabbitai bot May 12, 2024
@janosh
Copy link
Member

janosh commented May 12, 2024

using the attribute directly instead of the property method doesn't add to the call stack and so saves small amount of work.

but you can't lazy-load an attribute like you can a property. same with caching. that's probably what "Properties lazily generated for efficiency. " is referring to

In 710cb91, access to Lattice._pbc has been redirected to Lattice.pbc, with a setter method. Wondering if there is any reason for the previous implementation?

not that i'm aware. your way makes more sense imo

@janosh
Copy link
Member

janosh commented May 12, 2024

2. Is the following needed by anything?

if __name__ == "__main__":
for xc in LibxcFunc:
print(xc)

i think that's safe to remove

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented May 12, 2024

Thanks for the input. It's very helpful. I would do them in a future PR.

Also worth north is the regen_libxcfunc.py dev script seems to depend on the comments to work properly. This feels so weird and might be good to improve this as well.

# [1] insert new enum values in list
start = lines.index("#begin_include_dont_touch\n")
stop = lines.index("#end_include_dont_touch\n")
lines.insert(stop, enum_list)
del lines[start + 1 : stop]

@unique
class LibxcFunc(Enum):
"""Enumerator with the identifiers. This object is used by Xcfunc
declared in xcfunc.py to create an internal representation of the XC functional.
This is a low level object, client code should not interact with LibxcFunc directly
but use the API provided by Xcfunc.
"""
# Warning: the following header is required by `regen_libxcfunc.py`
# begin_include_dont_touch
LDA_C_1D_CSC = 18
LDA_C_1D_LOOS = 26
LDA_C_2D_AMGB = 15
LDA_C_2D_PRM = 16
LDA_C_GOMBAS = 24
LDA_C_HL = 4
LDA_C_GL = 5
LDA_C_vBH = 17
LDA_C_ML1 = 22
LDA_C_ML2 = 23
LDA_C_PW = 12
LDA_C_PW_MOD = 13
LDA_C_OB_PW = 14
LDA_C_PW_RPA = 25
LDA_C_PZ = 9
LDA_C_PZ_MOD = 10
LDA_C_OB_PZ = 11
LDA_C_RC04 = 27
LDA_C_RPA = 3
LDA_C_VWN = 7
LDA_C_VWN_1 = 28
LDA_C_VWN_2 = 29
LDA_C_VWN_3 = 30
LDA_C_VWN_4 = 31
LDA_C_VWN_RPA = 8
LDA_C_WIGNER = 2
LDA_K_TF = 50
LDA_K_LP = 51
LDA_X = 1
LDA_C_XALPHA = 6
LDA_X_1D = 21
LDA_X_2D = 19
LDA_XC_KSDT = 259
LDA_XC_TETER93 = 20
LDA_XC_ZLP = 43
GGA_C_AM05 = 135
GGA_C_FT97 = 88
GGA_C_LM = 137
GGA_C_LYP = 131
GGA_C_OP_B88 = 87
GGA_C_OP_PBE = 86
GGA_C_OP_G96 = 85
GGA_C_OP_PW91 = 262
GGA_C_OP_XALPHA = 84
GGA_C_OPTC = 200
GGA_C_P86 = 132
GGA_C_PBE = 130
GGA_C_PBE_SOL = 133
GGA_C_XPBE = 136
GGA_C_PBE_JRGX = 138
GGA_C_RGE2 = 143
GGA_C_APBE = 186
GGA_C_SPBE = 89
GGA_C_REGTPSS = 83
GGA_C_ZPBESOL = 63
GGA_C_PBEINT = 62
GGA_C_ZPBEINT = 61
GGA_C_PBELOC = 246
GGA_C_BGCP = 39
GGA_C_PBEFE = 258
GGA_C_PW91 = 134
GGA_C_Q2D = 47
GGA_C_SOGGA11 = 152
GGA_C_SOGGA11_X = 159
GGA_C_TCA = 100
GGA_C_REVTCA = 99
GGA_C_WI0 = 153
GGA_C_WI = 148
GGA_C_WL = 147
GGA_K_DK = 516
GGA_K_PERDEW = 517
GGA_K_VSK = 518
GGA_K_VJKS = 519
GGA_K_ERNZERHOF = 520
GGA_K_MEYER = 57
GGA_K_OL1 = 512
GGA_X_OL2 = 183
GGA_K_OL2 = 513
GGA_K_PEARSON = 511
GGA_K_TFVW = 52
GGA_K_VW = 500
GGA_K_GE2 = 501
GGA_K_GOLDEN = 502
GGA_K_YT65 = 503
GGA_K_BALTIN = 504
GGA_K_LIEB = 505
GGA_K_ABSP1 = 506
GGA_K_ABSP2 = 507
GGA_K_GR = 508
GGA_K_LUDENA = 509
GGA_K_GP85 = 510
GGA_X_2D_B86 = 128
GGA_X_2D_B86_MGC = 124
GGA_X_2D_B88 = 127
GGA_X_2D_PBE = 129
GGA_X_AIRY = 192
GGA_X_LAG = 193
GGA_X_AK13 = 56
GGA_X_AM05 = 120
GGA_X_B86 = 103
GGA_X_B86_MGC = 105
GGA_X_B86_R = 41
GGA_X_B88 = 106
GGA_X_OPTB88_VDW = 139
GGA_X_MB88 = 149
GGA_K_LLP = 522
GGA_K_FR_B88 = 514
GGA_K_THAKKAR = 523
GGA_X_BAYESIAN = 125
GGA_X_BPCCAC = 98
GGA_X_C09X = 158
GGA_X_CAP = 270
GGA_X_DK87_R1 = 111
GGA_X_DK87_R2 = 112
GGA_X_EV93 = 35
GGA_X_FT97_A = 114
GGA_X_FT97_B = 115
GGA_X_G96 = 107
GGA_X_HCTH_A = 34
GGA_X_HERMAN = 104
GGA_X_HJS_PBE = 525
GGA_X_HJS_PBE_SOL = 526
GGA_X_HJS_B88 = 527
GGA_X_HJS_B97X = 528
GGA_X_HJS_B88_V2 = 46
GGA_X_HTBS = 191
GGA_X_ITYH = 529
GGA_X_KT1 = 145
GGA_XC_KT2 = 146
GGA_X_LB = 160
GGA_X_LBM = 182
GGA_X_LG93 = 113
GGA_X_LV_RPW86 = 58
GGA_X_MPBE = 122
GGA_X_N12 = 82
GGA_X_GAM = 32
GGA_X_OPTX = 110
GGA_X_PBE = 101
GGA_X_PBE_R = 102
GGA_X_PBE_SOL = 116
GGA_X_XPBE = 123
GGA_X_PBE_JSJR = 126
GGA_X_PBEK1_VDW = 140
GGA_X_RGE2 = 142
GGA_X_APBE = 184
GGA_X_PBEINT = 60
GGA_X_PBE_TCA = 59
GGA_X_LAMBDA_LO_N = 45
GGA_X_LAMBDA_CH_N = 44
GGA_X_LAMBDA_OC2_N = 40
GGA_X_PBE_MOL = 49
GGA_X_BGCP = 38
GGA_X_PBEFE = 265
GGA_K_APBE = 185
GGA_K_REVAPBE = 55
GGA_K_TW1 = 187
GGA_K_TW2 = 188
GGA_K_TW3 = 189
GGA_K_TW4 = 190
GGA_K_APBEINT = 54
GGA_K_REVAPBEINT = 53
GGA_X_PBEA = 121
GGA_X_PW86 = 108
GGA_X_RPW86 = 144
GGA_K_FR_PW86 = 515
GGA_X_PW91 = 109
GGA_X_MPW91 = 119
GGA_K_LC94 = 521
GGA_X_Q2D = 48
GGA_X_RPBE = 117
GGA_X_SFAT = 530
GGA_X_SOGGA11 = 151
GGA_X_SSB_SW = 90
GGA_X_SSB = 91
GGA_X_SSB_D = 92
GGA_X_VMT_PBE = 71
GGA_X_VMT_GE = 70
GGA_X_VMT84_PBE = 69
GGA_X_VMT84_GE = 68
GGA_X_WC = 118
GGA_X_WPBEH = 524
GGA_XC_XLYP = 166
GGA_XC_PBE1W = 173
GGA_XC_MPWLYP1W = 174
GGA_XC_PBELYP1W = 175
GGA_XC_B97_D = 170
GGA_XC_HCTH_93 = 161
GGA_XC_HCTH_120 = 162
GGA_XC_HCTH_147 = 163
GGA_XC_HCTH_407 = 164
GGA_C_HCTH_A = 97
GGA_XC_B97_GGA1 = 96
GGA_XC_HCTH_P14 = 95
GGA_XC_HCTH_P76 = 94
GGA_XC_HCTH_407P = 93
GGA_C_N12 = 80
GGA_C_N12_SX = 79
GGA_C_GAM = 33
GGA_XC_EDF1 = 165
GGA_X_OPTPBE_VDW = 141
GGA_XC_MOHLYP = 194
GGA_XC_MOHLYP2 = 195
GGA_X_SOGGA = 150
GGA_XC_OBLYP_D = 67
GGA_XC_OPWLYP_D = 66
GGA_XC_OPBE_D = 65
GGA_XC_TH_FL = 196
GGA_XC_TH_FC = 197
GGA_XC_TH_FCFO = 198
GGA_XC_TH_FCO = 199
GGA_XC_TH1 = 154
GGA_XC_TH2 = 155
GGA_XC_TH3 = 156
GGA_XC_TH4 = 157
GGA_XC_VV10 = 255
HYB_GGA_XC_CAP0 = 477
HYB_GGA_X_N12_SX = 81
HYB_GGA_X_SOGGA11_X = 426
HYB_GGA_XC_B97 = 407
HYB_GGA_XC_B97_1 = 408
HYB_GGA_XC_B97_2 = 410
HYB_GGA_XC_B97_K = 413
HYB_GGA_XC_B97_3 = 414
HYB_GGA_XC_SB98_1a = 420
HYB_GGA_XC_SB98_1b = 421
HYB_GGA_XC_SB98_1c = 422
HYB_GGA_XC_SB98_2a = 423
HYB_GGA_XC_SB98_2b = 424
HYB_GGA_XC_SB98_2c = 425
HYB_GGA_XC_WB97 = 463
HYB_GGA_XC_WB97X = 464
HYB_GGA_XC_WB97X_V = 466
HYB_GGA_XC_WB97X_D = 471
HYB_GGA_XC_B97_1p = 266
HYB_GGA_XC_LC_VV10 = 469
HYB_GGA_XC_B1WC = 412
HYB_GGA_XC_B1LYP = 416
HYB_GGA_XC_B1PW91 = 417
HYB_GGA_XC_mPW1PW = 418
HYB_GGA_XC_mPW1K = 405
HYB_GGA_XC_BHANDH = 435
HYB_GGA_XC_BHANDHLYP = 436
HYB_GGA_XC_MPWLYP1M = 453
HYB_GGA_XC_B3PW91 = 401
HYB_GGA_XC_B3LYP = 402
HYB_GGA_XC_B3LYP5 = 475
HYB_GGA_XC_B3P86 = 403
HYB_GGA_XC_MPW3PW = 415
HYB_GGA_XC_MPW3LYP = 419
HYB_GGA_XC_MB3LYP_RC04 = 437
HYB_GGA_XC_REVB3LYP = 454
HYB_GGA_XC_B3LYPs = 459
HYB_GGA_XC_CAM_B3LYP = 433
HYB_GGA_XC_TUNED_CAM_B3LYP = 434
HYB_GGA_XC_CAMY_B3LYP = 470
HYB_GGA_XC_CAMY_BLYP = 455
HYB_GGA_XC_EDF2 = 476
HYB_GGA_XC_HSE03 = 427
HYB_GGA_XC_HSE06 = 428
HYB_GGA_XC_LRC_WPBEH = 465
HYB_GGA_XC_LRC_WPBE = 473
HYB_GGA_XC_HJS_PBE = 429
HYB_GGA_XC_HJS_PBE_SOL = 430
HYB_GGA_XC_HJS_B88 = 431
HYB_GGA_XC_HJS_B97X = 432
HYB_GGA_XC_LCY_BLYP = 468
HYB_GGA_XC_LCY_PBE = 467
HYB_GGA_XC_O3LYP = 404
HYB_GGA_XC_X3LYP = 411
HYB_GGA_XC_PBEH = 406
HYB_GGA_XC_PBE0_13 = 456
HYB_GGA_XC_HPBEINT = 472
MGGA_XC_TPSSLYP1W = 242
MGGA_C_BC95 = 240
MGGA_C_CC06 = 229
MGGA_C_CS = 72
MGGA_C_M08_HX = 78
MGGA_C_M08_SO = 77
MGGA_C_M11 = 76
MGGA_C_M11_L = 75
MGGA_C_MN12_L = 74
MGGA_C_MN12_SX = 73
MGGA_C_MN15_L = 261
MGGA_C_MN15 = 269
MGGA_C_PKZB = 239
MGGA_C_TPSS = 231
MGGA_C_REVTPSS = 241
MGGA_C_TPSSLOC = 247
MGGA_C_SCAN = 267
MGGA_C_M05 = 237
MGGA_C_M05_2X = 238
MGGA_C_VSXC = 232
MGGA_C_M06_L = 233
MGGA_C_M06_HF = 234
MGGA_C_M06 = 235
MGGA_C_M06_2X = 236
MGGA_C_DLDF = 37
MGGA_X_2D_PRHG07 = 210
MGGA_X_2D_PRHG07_PRP10 = 211
MGGA_X_BR89 = 206
MGGA_X_BJ06 = 207
MGGA_X_TB09 = 208
MGGA_X_RPP09 = 209
MGGA_X_GVT4 = 204
MGGA_X_LTA = 201
MGGA_X_M05 = 214
MGGA_X_M05_2X = 215
MGGA_X_M06_2X = 218
MGGA_X_M06_L = 203
MGGA_X_M06_HF = 216
MGGA_X_M06 = 217
MGGA_X_M08_HX = 219
MGGA_X_M08_SO = 220
MGGA_X_M11 = 225
MGGA_X_M11_L = 226
MGGA_X_MBEEF = 249
MGGA_X_MBEEFVDW = 250
MGGA_X_MK00 = 230
MGGA_X_MK00B = 243
MGGA_X_MN12_L = 227
MGGA_X_MN15_L = 260
MGGA_X_MS0 = 221
MGGA_X_MS1 = 222
MGGA_X_MS2 = 223
MGGA_X_MVS = 257
MGGA_X_PKZB = 213
MGGA_X_SCAN = 263
MGGA_X_TAU_HCTH = 205
MGGA_X_TPSS = 202
MGGA_X_MODTPSS = 245
MGGA_X_REVTPSS = 212
MGGA_X_BLOC = 244
MGGA_XC_B97M_V = 254
MGGA_XC_OTPSS_D = 64
MGGA_XC_ZLP = 42
HYB_MGGA_X_MVSH = 474
HYB_MGGA_XC_M05 = 438
HYB_MGGA_XC_M05_2X = 439
HYB_MGGA_XC_B88B95 = 440
HYB_MGGA_XC_B86B95 = 441
HYB_MGGA_XC_PW86B95 = 442
HYB_MGGA_XC_BB1K = 443
HYB_MGGA_XC_MPW1B95 = 445
HYB_MGGA_XC_MPWB1K = 446
HYB_MGGA_XC_X1B95 = 447
HYB_MGGA_XC_XB1K = 448
HYB_MGGA_XC_M06_HF = 444
HYB_MGGA_XC_M06 = 449
HYB_MGGA_XC_M06_2X = 450
HYB_MGGA_XC_PW6B95 = 451
HYB_MGGA_XC_PWB6K = 452
HYB_MGGA_XC_TPSSH = 457
HYB_MGGA_XC_REVTPSSH = 458
HYB_MGGA_X_DLDF = 36
HYB_MGGA_XC_M08_HX = 460
HYB_MGGA_XC_M08_SO = 461
HYB_MGGA_XC_M11 = 462
HYB_MGGA_X_MN12_SX = 248
HYB_MGGA_X_MN15 = 268
HYB_MGGA_X_MS2H = 224
HYB_MGGA_X_SCAN0 = 264
HYB_MGGA_XC_WB97M_V = 531
# end_include_dont_touch

@janosh janosh added linting Linting and quality assurance types Type all the things labels May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linting Linting and quality assurance types Type all the things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants