Chargemol autodownloads#3474
Conversation
| # mock getenv to simulate that DDEC6_ATOMIC_DENSITIES_DIR is not set | ||
| monkeypatch.setattr(os, "getenv", lambda *args, **kwargs: None) | ||
| def fake_download(self, version: str = "latest", verbose: bool = True) -> None: | ||
| extraction_path = "~/.cache/pymatgen/ddec" |
There was a problem hiding this comment.
Be careful with path handling. You don't want to impose a given OS.
I recommend using the pathlib library instead.
There was a problem hiding this comment.
Actually, forward slashes work under windows. I've only encountered a few arcane cases where they cause problems. See the pmg test suite. Runs on Windows in GHA and it's all forward slashes.
There was a problem hiding this comment.
The ~ is not valid on Windows, although I guess you get out of that pickle by doing os.path.expanduser.
There was a problem hiding this comment.
Doing os.path.expanduser(extraction_path) expands to 'C:\\Users\\asros/.cache/pymatgen/ddec' which will work with os.makedir since os is smart. So, tests will pass, but you're playing with fire a bit... 😅
Guess this is more personal preference because I'm a Windows user tired of opening PRs to fix paths, although it's indeed fine here after closer inspection.
There was a problem hiding this comment.
Yeah, i'm running a live experiment. So far it's only caused issues 2 or 3 times and they were niche. Feel like so far the readability gain from not using os.path.join was well worth it. Need to reconsider if sth non-niche comes up.
There was a problem hiding this comment.
Feel free to ignore these comments then @chiang-yuan. Good to know, @janosh.
There was a problem hiding this comment.
Thanks @Andrew-S-Rosen . I actually prefer pathlib since it is a newer standard over os.path. I was just following what we already have in the code 😉 Will change it whenever appropriate :)
There was a problem hiding this comment.
Seems it doesn't really matter :)
There was a problem hiding this comment.
while pathlib has some nice features, i find it a bit too OOP with resulting perf overhead compared to os.path (see e.g. https://youtu.be/tFrh9hKMS6Y). although admittedly the speed diff doesn't matter in practice.
| fake_zip_path = os.path.expanduser(f"{extraction_path}/fake_file.zip") | ||
| fake_folder_path = os.path.expanduser(f"{extraction_path}/atomic_densities") |
There was a problem hiding this comment.
Same comments here regarding path handling. Do not hard-code slashes. There are more below. You get the idea.
|
@janosh I use monkeypatch to mock the class method directly (rather than through module namespace) and add |
chiang-yuan
force-pushed
the
chargemol-auto-download
branch
from
65eb4fe to
8090776
Compare
janosh
left a comment
janosh
left a comment
There was a problem hiding this comment.
@chiang-yuan thanks! i think there are a lot of unintended white space/indentation changes from trailing commas in this PR?
There was a problem hiding this comment.
can you revert the trailing comma insertions in this file and tests/command_line/test_chargemol_caller.py? makes it hard to spot meaningful changes
There was a problem hiding this comment.
hmmm sorry let me check... perhaps my ruff is not working correctly
|
@chiang-yuan I would like to merge this, but no tests are being run for some reason. Only pre-commit was run. Pls make sure the test suite is run properly. Thanks. |
|
i'll close this for the same reason as #3465 (i.e. stalled). feel free to re-open if you want to resolve merge conflicts @chiang-yuan. |
4 participants
Try to close #3465
Checklist
ruff.mypy.If applicable, new classes/functions/modules haveduecredit@due.dcitedecorators to reference relevant papers by DOI (example)Tip: Install
pre-commithooks to auto-check types and linting before every commit: