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

Fix TypeError: can only join an iterable with AECCAR in VolumetricData.write_file #3343

Merged
merged 9 commits into from
Sep 30, 2023

Conversation

chiang-yuan
Copy link
Contributor

@chiang-yuan chiang-yuan commented Sep 20, 2023

Context: I was trying to download AECCARs from aws and save them locally as vasp compatible format. CHGCARs can pass successfully but AECCAR will yield error when aeccar.write_file like following

mp_id = 'mp-1523380'
prefix = 'aeccar0s'
suffix = prefix.rstrip("s")

fpath = f'{mp_id}-{suffix}.json.gz'

with zopen(fpath) as f:
    json_string = f.read()
    data = json.loads(json_string, cls=MontyDecoder)['data']
    data.write_file(f'{mp_id}.{suffix.upper()}')

Modify io/vasp/outputs.py to do the Iterable check first and only write to file when self.data_aug.get(data_type, []) is not empty

Checklist

  • ruff
  • mypy
  • pre-commit

@chiang-yuan
Copy link
Contributor Author

tag @tschaume you might be interested to comment since I mentioned this bug to you 😄

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 @chiang-yuan! Could you mention the error type and message you're seeing? We'll also need a test for this. You can maybe use tests/files/bader/AECCAR0.gz

@janosh janosh added needs testing PRs that are not ready to merge due to lacking tests io Input/output functionality fix Bug fix PRs vasp Vienna Ab initio Simulation Package labels Sep 20, 2023
@chiang-yuan
Copy link
Contributor Author

The error I received is

-> 3585 f.write("".join(self.data_aug.get(data_type, [])))

TypeError: can only join an iterable

@janosh this is a file output bug, so do you want me to check if the file looks identical to itself after read in and output?

@janosh janosh changed the title bugfix for aeccars write_file Fix TypeError: can only join an iterable with AECCAR in VolumetricData.write_file Sep 21, 2023
@janosh
Copy link
Member

janosh commented Sep 21, 2023

@janosh this is a file output bug, so do you want me to check if the file looks identical to itself after read in and output?

Sounds good! Just writing to disk would be enough to test this fix but checking for equality after re-read is even better.

@chiang-yuan
Copy link
Contributor Author

Test added :) @janosh

f.write("".join(self.data_aug.get(data_type, [])))

data = self.data_aug.get(data_type, [])
if isinstance(data, Iterable):
Copy link
Member

Choose a reason for hiding this comment

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

Why is it ok here not to write to file if data is not Iterable?

Copy link
Contributor Author

@chiang-yuan chiang-yuan Sep 30, 2023

Choose a reason for hiding this comment

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

If data_aug is empty and not Iterable, join will give the error.

Copy link
Member

Choose a reason for hiding this comment

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

But do we want to write an empty file in that case? Having no file after calling write_file sounds like unexpected behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the partial of the file (data_aug) will not be written if it is empty. The rest of the file will still be there.

aeccar0_test = Chgcar.from_file(f"{TEST_FILES_DIR}/bader/AECCAR0.gz")
aeccar0_test.write_file("AECCAR0_test")
aeccar0_read = Chgcar.from_file("AECCAR0_test")
np.allclose(aeccar0_test.data["total"], aeccar0_read.data["total"])
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be asserted. Just calling np.allclose doesn't fail the test if the check fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh!!! Thanks for the good catch. I missed that.

Copy link
Contributor Author

@chiang-yuan chiang-yuan left a comment

Choose a reason for hiding this comment

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

Thanks for the help @janosh! All look good to me 😃

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 for the test! 👍

@janosh janosh removed the needs testing PRs that are not ready to merge due to lacking tests label Sep 30, 2023
@janosh janosh merged commit 650df90 into materialsproject:master Sep 30, 2023
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix PRs io Input/output functionality vasp Vienna Ab initio Simulation Package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants