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

Issue stronger warning if bader is run without the AECCARs #3458

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

janosh
Copy link
Member

@janosh janosh commented Nov 5, 2023

Closes #3448.

@janosh janosh added fix Bug fix PRs analysis Concerning pymatgen.analysis cli Command line interface labels Nov 5, 2023
@janosh janosh changed the title Issue strong warning if bader is run without the AECCARs Issue stronger warning if bader is run without the AECCARs Nov 6, 2023
@janosh janosh merged commit abd9893 into master Nov 6, 2023
22 checks passed
@janosh janosh deleted the fix-gh-3448 branch November 6, 2023 16:26
Copy link
Contributor

@JiQi535 JiQi535 left a comment

Choose a reason for hiding this comment

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

I'm quite sure that this PR is causing the "bader_analysis_from_path" method to fail for any of its usage. I'm confused how could this PR pass the tests without failing.

The problematic line is line 455 in this PR and now line 451 in the latest pymatgen. The method is now looking for CHGCAR in the "Could not find CHGCAR!" folder path...

I think the tests and this code should be corrected.

def _get_filepath(filename, path=path, suffix=suffix):
paths = glob(f"{path}/{filename}{suffix}*")
if len(paths) == 0:
return None
if len(paths) > 1:
# using reverse=True because, if multiple files are present,
# they likely have suffixes 'static', 'relax', 'relax2', etc.
# and this would give 'static' over 'relax2' over 'relax'
# however, better to use 'suffix' kwarg to avoid this!
paths.sort(reverse=True)
warnings.warn(f"Multiple files detected, using {os.path.basename(path)}")
return paths[0]
chgcar_path = _get_filepath("CHGCAR", "Could not find CHGCAR!")
chgcar = Chgcar.from_file(chgcar_path)

@janosh
Copy link
Member Author

janosh commented Feb 17, 2024

@JiQi535 you're right, thanks for reporting. the reason this wasn't caught is that the only test for bader_analysis_from_path is skipped

assert_allclose(val, val_from_path, atol=1e-5)
def test_automatic_runner(self):
pytest.skip("raises RuntimeError: bader exits with return code 24")
summary = bader_analysis_from_path(f"{TEST_FILES_DIR}/bader")
"""
Reference summary dict (with bader 1.0)
summary_ref = {
'magmom': [4.298761, 4.221997, 4.221997, 3.816685, 4.221997, 4.298763, 0.36292,
0.370516, 0.36292, 0.36292, 0.36292, 0.36292, 0.36292, 0.370516],
'min_dist': [0.835789, 0.92947, 0.92947, 0.973007, 0.92947, 0.835789, 0.94067,
0.817381, 0.94067, 0.94067, 0.94067, 0.94067, 0.94067, 0.817381],
'vacuum_charge': 0.0,
'vacuum_volume': 0.0,
'atomic_volume': [9.922887, 8.175158, 8.175158, 9.265802, 8.175158, 9.923233, 12.382546,
12.566972, 12.382546, 12.382546, 12.382546, 12.382546, 12.382546, 12.566972],

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analysis Concerning pymatgen.analysis cli Command line interface fix Bug fix PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Clear warning if Bader is run without the AECCARs as reference
2 participants