Skip to content

Fix consideration of translation symmetry for some (extremely rare) edge cases in LobsterEnv#4148

Merged
shyuep merged 76 commits intomaterialsproject:masterfrom
JaGeo:fix_icohp
Mar 10, 2026
Merged

Fix consideration of translation symmetry for some (extremely rare) edge cases in LobsterEnv#4148
shyuep merged 76 commits intomaterialsproject:masterfrom
JaGeo:fix_icohp

Conversation

@JaGeo
Copy link
Member

@JaGeo JaGeo commented Oct 30, 2024 •
edited
Loading

Summary

  • completely solve the translation question (i.e., figuring out the direction and origin)
  • add a test case where this plays a role

@JaGeo JaGeo requested review from mkhorton and shyuep as code owners October 30, 2024 08:00
@JaGeo JaGeo marked this pull request as draft October 30, 2024 08:00
JaGeo
JaGeo commented Jan 14, 2025
View reviewed changes
src/pymatgen/io/lobster/lobsterenv.py Outdated
Comment on lines +997 to +1004
or (
copied_translations_from_ICOHPs[dist_idx][0]
== translations_by_distance[neigh_idx][0]
and copied_translations_from_ICOHPs[dist_idx][1]
== translations_by_distance[neigh_idx][1]
and copied_translations_from_ICOHPs[dist_idx][2]
== translations_by_distance[neigh_idx][2]
)
Copy link
Member Author

@JaGeo JaGeo Jan 14, 2025

Choose a reason for hiding this comment

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

We could raise a warning if this fall-back option is taken. I think with updating to reading the CONTCAR, most issues should be fixed, however.

Copy link
Contributor

@kaueltzen kaueltzen Jan 14, 2026

Choose a reason for hiding this comment

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

done

JaGeo and others added 22 commits January 24, 2025 18:53
@JaGeo
Merge branch 'master' of github.com:JaGeo/pymatgen into fix_icohp
0d5ff55
@JaGeo
merge current mastr
e7b558a
@pre-commit-ci
pre-commit auto-fixes
8a8003e
@naik-aakash
Merge branch 'materialsproject:master' into fix_icohp
a922572
@JaGeo
add new test data
aa1fda5
@JaGeo
merge conflict
bc1aa7b
@pre-commit-ci
pre-commit auto-fixes
9ac8003
@JaGeo
fix some more
527e675
@JaGeo
Merge branch 'fix_icohp' of github.com:JaGeo/pymatgen into fix_icohp
8af3352
@pre-commit-ci
pre-commit auto-fixes
df3589a
@JaGeo
fix nearly everything except for two inter neighbor things
5520121
@JaGeo
Merge branch 'fix_icohp' of github.com:JaGeo/pymatgen into fix_icohp
5921ed8
@pre-commit-ci
pre-commit auto-fixes
8c27943
@JaGeo
add more fixes
e908e47
@JaGeo
add more fixes
17037c2
@pre-commit-ci
pre-commit auto-fixes
5436d40
@JaGeo
fix the just introduced bug and add a warning for very rare cases tha…
cfdd0ea 
…t we do not cover yet
@JaGeo
fix the just introduced bug and add a warning for very rare cases tha…
bf1b7fd 
…t we do not cover yet
@pre-commit-ci
pre-commit auto-fixes
47aca41
@JaGeo
fix next test by adapting cutoff
53a2a67
@JaGeo
Merge branch 'fix_icohp' of github.com:JaGeo/pymatgen into fix_icohp
9154b3c
@pre-commit-ci
pre-commit auto-fixes
413ed2f
@JaGeo JaGeo mentioned this pull request Feb 25, 2026
Merged
@naik-aakash
Copy link
Contributor

naik-aakash commented Feb 25, 2026 •
edited
Loading

Hi @shyuep and @JaGeo , based on the issue pointed here #4595 (comment)

I moved the lobsterenv.py module to the analysis module in this PR and added a deprecation warning for older class imports pointing to the new location. Maybe this helps with moving lobster.io modules to pymatgen-core, since other Lobster parsers do not depend on local_env or chemenv, I think. Correct me in case I overlooked anything

naik-aakash and others added 2 commits February 25, 2026 16:45
@naik-aakash
include future module yaml
e7c6e59
@naik-aakash
Merge branch 'master' into fix_icohp
82f45cd 
Signed-off-by: Aakash Ashok Naik <91958822+naik-aakash@users.noreply.github.com>
@naik-aakash naik-aakash mentioned this pull request Mar 3, 2026
Open
@naik-aakash
Copy link
Contributor

naik-aakash commented Mar 4, 2026

Hi @shyuep, thanks for already moving the lobster/io modules to pymatgen-core. Currently, I notice only the cohp module in electronic_structure is still in main pymatgen. Is there any dependency issue preventing the move of this module?

And I can't figure out why the tests in this PR aren't using the changes I made in this module and are failing with an attribute error; it seems they are not using the modified cohp module as expected. Not sure if it has something to do with the split of the electronic_structure module itself or in the CI workflow.

After resolving this and once the test-files PR is merged, hopefully lobster-related modules can be fully functional.

@JaGeo
Copy link
Member Author

JaGeo commented Mar 4, 2026

Just to add this: we are aware that the split between pymatgen-core and pymatgen is currently under discussion. However, we wanted to leave the comment for potential future resolution of the issue.

@naik-aakash naik-aakash force-pushed the fix_icohp branch 3 times, most recently from 893393c to c48a2eb Compare March 9, 2026 20:08
@naik-aakash
Copy link
Contributor

naik-aakash commented Mar 10, 2026

Hi @shyuep, I have made the necessary changes here. 😃

@shyuep shyuep merged commit c58dfe4 into materialsproject:master Mar 10, 2026
5 of 6 checks passed
@shyuep
Copy link
Member

shyuep commented Mar 10, 2026

Great thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shyuep shyuep Awaiting requested review from shyuep shyuep is a code owner

@mkhorton mkhorton Awaiting requested review from mkhorton mkhorton is a code owner

1 more reviewer

@kaueltzen kaueltzen kaueltzen left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@JaGeo @naik-aakash @mkhorton @shyuep @kaueltzen