speed up `bader_caller` and `chargemol_caller` #3192

chiang-yuan · 2023-07-29T00:01:06Z

In the original implementation, CHGCAR is copied into temporary folder by monty.zopen. Since CHGCAR is usually very large, this makes bader analysis calling from pymatgen extremely slow.

this might close #2487

fix 1: use monty.tempfiie.ScratchdDr context manager
fix 2: do not copy large files like CHGCAR into temporary folder but read them in place instead
fix 3: use os.symlink to link larges files for chargemol_caller

Checklist

Google format doc strings added. Check with ruff.
Type annotations included. Check with mypy.
Tests added for new features/fixes.

…ile.ScratchdDr` context manager

Andrew-S-Rosen · 2023-07-29T02:40:04Z

Thank you so much for doing this! It has been on my backlog for a very long time.

Andrew-S-Rosen · 2023-07-29T02:41:40Z

You don't need to add it in this PR (unless you're feeling ambitious!), but it's likely that an analogous fix will solve similar issues with the chargemol_caller.

pymatgen/pymatgen/command_line/chargemol_caller.py

Lines 181 to 189 in 0b7607a

    
           with ScratchDir("."): 
        
               with zopen(self._chgcarpath, "rt") as f_in, open("CHGCAR", "w") as f_out: 
        
                   shutil.copyfileobj(f_in, f_out) 
        
               with zopen(self._potcarpath, "rt") as f_in, open("POTCAR", "w") as f_out: 
        
                   shutil.copyfileobj(f_in, f_out) 
        
               with zopen(self._aeccar0path, "rt") as f_in, open("AECCAR0", "w") as f_out: 
        
                   shutil.copyfileobj(f_in, f_out) 
        
               with zopen(self._aeccar2path, "rt") as f_in, open("AECCAR2", "w") as f_out: 
        
                   shutil.copyfileobj(f_in, f_out)

chiang-yuan · 2023-07-30T02:03:53Z

@arosen93 thanks for pointing out chagemol as well! It actually requires little modification so I guess I can try to solve them both.

Andrew-S-Rosen · 2023-07-30T02:05:07Z

@chiang-yuan: I would certainly be very grateful if you did!!! 🙏

chiang-yuan · 2023-07-30T06:02:20Z

Chargemol needs to see all the input files in the same folder so the way around here is using symbolic link to point to large files . I also port the error message by bader and chagmol back to python RuntimeError.

I have used pytest to test test_bader_caller.py and test_chargemol_caller.py locally on Perlmutter and they all passed. If this change cannot pass pytest on github workflow, I don't know where is the problem then... Perhaps somehow bader and chagemol are not correctly installed on the pytest environment.

chiang-yuan · 2023-07-30T06:19:26Z

Looks like CHGCAR passed but cube failed. Maybe @janosh @arosen93 you will be interested in looking into this?

https://github.com/materialsproject/pymatgen/actions/runs/5704593631/job/15458352230?pr=3192#step:7:183

…t-bader

chiang-yuan · 2023-07-30T19:14:00Z

I found the problem. The code was reading compressed test files but somehow didn't raise error on Perlmutter. Modify the code to handle compressed files and decompress them to temp directory if needed. Not sure if this is the ideal solution since this falls back to copying data to temp folder again (but only for compressed file and folder, the uncompressed files are still read at their original place).

This will also affect when we want to compress all the test files. #2994

janosh

@chiang-yuan Thanks for taking this on! Very excited about fully reviving the BaderAnalysis in pymatgen. Left a few questions.

janosh · 2023-07-31T02:31:36Z

pymatgen/command_line/bader_caller.py

 import subprocess
 import warnings
 from glob import glob
 from shutil import which
 from tempfile import TemporaryDirectory

 import numpy as np
-from monty.io import zopen
+from monty.tempfile import ScratchDir


I actually don't understand why the files need to be copied at all? Does bader modify them in place? Seems unlikely and even if, they should provide an option to disable that?

They don't! So what I did here is creating a temporary scratchdir to store the output files from bader. However, bader cannot read compressed file so at the bottom we need to decompress file if it is compressed and store it temporarily somewhere (I just put it in ScratchDir as well)

janosh · 2023-07-31T02:33:00Z

pymatgen/command_line/chargemol_caller.py

+                os.symlink(self._aeccar0path, "./AECCAR0")
+                os.symlink(self._aeccar2path, "./AECCAR2")
+            except OSError as e:
+                print(f"Error creating symbolic link: {e}")


Using os.symlink here seems strange. Why not read the files from where they are?

chargemol is different. It seems like here is no input arguments to indicate where the input files (e.g. CHGCAR AECCAR0) stored, so we need to all the input file in ScratchDir. To avoid copying, just use symbolic link can "pretend" there are in the same directory but actually they are just the shortcut linked to the file in the original place.

janosh · 2023-07-31T02:33:52Z

pymatgen/command_line/tests/test_bader_caller.py

@@ -78,6 +78,8 @@ def test_from_path(self):
        assert np.allclose(charge, charge0)
        if os.path.exists("CHGREF"):
            os.remove("CHGREF")
+        if os.path.exists(os.path.join(test_dir, "CHGREF")):
+            os.remove(os.path.join(test_dir, "CHGREF"))


Would probably be best to use the PymatgenTest.tmp_path fixture so we don't have to worry about manually cleaning up test-generated files.

Is PymatgenTest.tmp_path already implemented? If yes, I need some time to look into how to refactor this part. If not, I need more time 😂

Yes, right here

pymatgen/pymatgen/util/testing.py

Lines 50 to 54 in 71b7aec

@pytest.fixture(autouse=True) # make all tests run a in a temporary directory accessible via self.tmp_path

def _tmp_dir(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:

# https://pytest.org/en/latest/how-to/unittest.html#using-autouse-fixtures-and-accessing-other-fixtures

monkeypatch.chdir(tmp_path) # change to pytest-provided temporary directory

self.tmp_path = tmp_path

Thanks I am looking into it!

janosh · 2023-07-31T02:34:39Z

pymatgen/util/io_utils.py

@@ -17,6 +17,29 @@
 __date__ = "Sep 23, 2011"


+def decompress_file_to_path(fin_path, fout_path=None):


Isn't this quite similar to monty.io.zopen?

Actually is more similar to decompress_file in monty but the function won't return the path and delete the original compressed file. Created a helper function there as a fix

I assume @shyuep wouldn't mind modifying monty.shutil.decompress_file, esp. since the doc string is currently wrong: arg compression doesn't actually exist.

https://github.com/materialsvirtuallab/monty/blob/2b91561d2483f2bebfc185c4945dfae85008c439/monty/shutil.py#L98

We can make a PR to monty to have the function return the path.

@chiang-yuan Let's delete this function and use the changes in materialsvirtuallab/monty#536 instead.

@shyuep Can we have a monty release whenever we do the next pymatgen release?

janosh · 2023-08-07T18:55:42Z

Sorry, looks like this was waiting on me. I missed that. Just fixed the merge conflict. I'll submit the PR to monty to return filepath from decompress_file.

@chiang-yuan Any additional tests we might want to add?

janosh · 2023-08-07T19:08:03Z

pymatgen/util/io_utils.py

@@ -17,6 +17,29 @@
 __date__ = "Sep 23, 2011"


+def decompress_file_to_path(fin_path, fout_path=None):


@chiang-yuan Let's delete this function and use the changes in materialsvirtuallab/monty#536 instead.

@shyuep Can we have a monty release whenever we do the next pymatgen release?

codecov-commenter · 2023-08-07T19:30:07Z

Codecov Report

Patch coverage: 80.39% and project coverage change: -0.57% ⚠️

Comparison is base (1fa96f8) 74.57% compared to head (6f9b905) 74.01%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3192      +/-   ##
==========================================
- Coverage   74.57%   74.01%   -0.57%     
==========================================
  Files         230      230              
  Lines       69494    69496       +2     
  Branches    16166    16163       -3     
==========================================
- Hits        51824    51436     -388     
- Misses      14595    15019     +424     
+ Partials     3075     3041      -34

Files Changed	Coverage Δ
pymatgen/command_line/chargemol_caller.py	`61.56% <0.00%> (-0.29%)`	⬇️
pymatgen/command_line/bader_caller.py	`60.61% <88.09%> (-1.12%)`	⬇️
pymatgen/util/io_utils.py	`91.30% <100.00%> (+1.83%)`	⬆️

... and 6 files with indirect coverage changes

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

chiang-yuan · 2023-08-08T03:23:17Z

@janosh I made two modifications, 1) delete decompress_file_to_path and use monty instead 2) use PymatgenTest.tmp_path fixture.

The monty version of the test is still older version so the test is unsuccessful.

…th in err msg

E assert 6.395428 == 6.6136782 ± 1.0e-03 E comparison failed E Obtained: 6.395428 E Expected: 6.6136782 ± 1.0e-03 /home/runner/work/pymatgen/pymatgen/tests/command_line/test_bader_caller.py:37: AssertionError

… compressed version which can't happen twice in same directory

E assert 6.613678 == 6.395428 ± 1.0e-03 and fix mypy

janosh · 2023-08-08T19:16:21Z

Thanks @chiang-yuan! 👍 this is a big improvement. required a lot of tweaking but i'm glad we got it merged.

mkhorton · 2023-08-08T19:59:32Z

Also offering thanks for this :)

do not copy large files like CHGCAR, into tempfolder,use `monty.tempf…

cd14f3f

…ile.ScratchdDr` context manager

chiang-yuan marked this pull request as ready for review July 29, 2023 00:01

add chgref_filename back

00c2a9e

chargemol: use symbolic link to link large files to temporary folder

258c812

chiang-yuan changed the title ~~speed up bader_caller~~ speed up bader_caller and chargemol_caller Jul 30, 2023

chiang-yuan added 4 commits July 29, 2023 23:45

chargemol: use symbolic link to link large files to temporary folder

97c5b9d

Merge branch 'fast-bader' of github.com:chiang-yuan/pymatgen into fas…

9404d75

…t-bader

decompress file to temporary file only if neccessary

5b67238

clean code and revise docstring

611186a

clean code and revise docstring

b671e41

janosh suggested changes Jul 31, 2023

View reviewed changes

chiang-yuan requested a review from janosh July 31, 2023 21:30

janosh force-pushed the master branch from 6349dc1 to da318a2 Compare August 1, 2023 23:26

Merge branch 'master' into fast-bader

5ba0448

janosh mentioned this pull request Aug 7, 2023

Return path to decompressed file from decompress_file() materialsvirtuallab/monty#536

Merged

fix mypy

6f9b905

janosh suggested changes Aug 7, 2023

View reviewed changes

chiang-yuan and others added 3 commits August 7, 2023 18:24

use monty.shutil.decompress_file

393986d

Merge branch 'materialsproject:master' into fast-bader

959a420

use PymatgenTest.tmp_path for test

e50cf6a

janosh added 10 commits August 8, 2023 08:50

shorten bader + chargemol RuntimeError message, show chargemol exe pa…

a0b04c5

…th in err msg

f-str path construction

e1cb3c0

use self.tmp_path fixture directly (is already function scoped)

8926584

simplify file path handling

8e4f432

fix assert analysis.data[0]["charge"] == approx(6.6136782, abs=1e-3)

f3d0ebc

E assert 6.395428 == 6.6136782 ± 1.0e-03 E comparison failed E Obtained: 6.395428 E Expected: 6.6136782 ± 1.0e-03 /home/runner/work/pymatgen/pymatgen/tests/command_line/test_bader_caller.py:37: AssertionError

add type hints to bader_caller.py

ebca522

fix kwarg bader_exe_path (wasn't actually used)

7560b1c

use f-str formatting in atom_chgcars

5a5ac2e

fix test_from_path, was broken due to decompressing files deletes the…

2ccf9c4

… compressed version which can't happen twice in same directory

revert assert analysis.data[0]["charge"] == approx(6.395428, abs=1e-3)

c604025

E assert 6.613678 == 6.395428 ± 1.0e-03 and fix mypy

janosh merged commit 7c70c1a into materialsproject:master Aug 8, 2023
15 of 18 checks passed

Andrew-S-Rosen mentioned this pull request Sep 3, 2023

Crash in Bader analysis due to missing file #2634

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

speed up `bader_caller` and `chargemol_caller` #3192

speed up `bader_caller` and `chargemol_caller` #3192

chiang-yuan commented Jul 29, 2023 •

edited

Loading

Andrew-S-Rosen commented Jul 29, 2023

Andrew-S-Rosen commented Jul 29, 2023 •

edited

Loading

chiang-yuan commented Jul 30, 2023

Andrew-S-Rosen commented Jul 30, 2023

chiang-yuan commented Jul 30, 2023

chiang-yuan commented Jul 30, 2023

chiang-yuan commented Jul 30, 2023

janosh left a comment

janosh Jul 31, 2023

chiang-yuan Jul 31, 2023

janosh Jul 31, 2023

chiang-yuan Jul 31, 2023

janosh Jul 31, 2023 •

edited

Loading

chiang-yuan Jul 31, 2023 •

edited

Loading

janosh Jul 31, 2023

chiang-yuan Jul 31, 2023

janosh Jul 31, 2023

chiang-yuan Jul 31, 2023

janosh Jul 31, 2023

janosh Aug 7, 2023

janosh commented Aug 7, 2023

janosh Aug 7, 2023

codecov-commenter commented Aug 7, 2023 •

edited

Loading

chiang-yuan commented Aug 8, 2023 •

edited

Loading

janosh commented Aug 8, 2023

mkhorton commented Aug 8, 2023

	@pytest.fixture(autouse=True) # make all tests run a in a temporary directory accessible via self.tmp_path
	def _tmp_dir(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
	# https://pytest.org/en/latest/how-to/unittest.html#using-autouse-fixtures-and-accessing-other-fixtures
	monkeypatch.chdir(tmp_path) # change to pytest-provided temporary directory
	self.tmp_path = tmp_path

		@@ -17,6 +17,29 @@
		__date__ = "Sep 23, 2011"


		def decompress_file_to_path(fin_path, fout_path=None):

speed up bader_caller and chargemol_caller #3192

speed up bader_caller and chargemol_caller #3192

Conversation

chiang-yuan commented Jul 29, 2023 • edited Loading

Checklist

Andrew-S-Rosen commented Jul 29, 2023

Andrew-S-Rosen commented Jul 29, 2023 • edited Loading

chiang-yuan commented Jul 30, 2023

Andrew-S-Rosen commented Jul 30, 2023

chiang-yuan commented Jul 30, 2023

chiang-yuan commented Jul 30, 2023

chiang-yuan commented Jul 30, 2023

janosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janosh Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

chiang-yuan Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janosh commented Aug 7, 2023

Choose a reason for hiding this comment

codecov-commenter commented Aug 7, 2023 • edited Loading

Codecov Report

chiang-yuan commented Aug 8, 2023 • edited Loading

janosh commented Aug 8, 2023

mkhorton commented Aug 8, 2023

speed up `bader_caller` and `chargemol_caller` #3192

speed up `bader_caller` and `chargemol_caller` #3192

chiang-yuan commented Jul 29, 2023 •

edited

Loading

Andrew-S-Rosen commented Jul 29, 2023 •

edited

Loading

janosh Jul 31, 2023 •

edited

Loading

chiang-yuan Jul 31, 2023 •

edited

Loading

codecov-commenter commented Aug 7, 2023 •

edited

Loading

chiang-yuan commented Aug 8, 2023 •

edited

Loading