Skip to content

Remove reverse readline, test against NP1 and recover NumPy 1 dependency support#203

Merged
janosh merged 7 commits intoCederGroupHub:mainfrom
DanielYang59:fix-monty-readline
Sep 15, 2024
Merged

Remove reverse readline, test against NP1 and recover NumPy 1 dependency support#203
janosh merged 7 commits intoCederGroupHub:mainfrom
DanielYang59:fix-monty-readline

Conversation

@DanielYang59
Copy link
Copy Markdown
Contributor

@DanielYang59 DanielYang59 commented Sep 15, 2024 •
edited
Loading

Summary

I'm not seeing much point in using reverse_readline as all lines are read, and reversed immediately:

chgnet/chgnet/utils/vasp_utils.py

Lines 61 to 67 in 11ff513

charge, mag_x, mag_y, mag_z, header, all_lines = [], [], [], [], [], []
for line in reverse_readfile(outcar_path):
clean = line.strip()
all_lines.append(clean)
all_lines.reverse()

@DanielYang59 DanielYang59 mentioned this pull request Sep 15, 2024
Merged
3 tasks
@janosh janosh added pkg Package fix Bug fix PRs labels Sep 15, 2024
@DanielYang59 DanielYang59 marked this pull request as ready for review September 15, 2024 13:04
@DanielYang59 DanielYang59 changed the title test monty fix for reverse readline Remove reverse readline, test again NP1 and recover NumPy 1 dependency support Sep 15, 2024
@DanielYang59
Copy link
Copy Markdown
Contributor Author

DanielYang59 commented Sep 15, 2024 •
edited
Loading

@janosh I believe this one is ready for review. I completely removed usage of reverse_readline because that OUTCAR file is read in entirely and reversed right after read in, as such I'm not seeing much point to use reverse_readline.

The reverse readers from monty is much slower than the built in readline(understandably because that function from monty is 11 years old and was never updated), see the benchmark in materialyzeai/monty#712 (comment). I would expect reverse readers to be more RAM-efficient though (but still bring no benefit to the use case here).

janosh
janosh approved these changes Sep 15, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@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.

good thing you noticed. strange that it read the file in reverse only to reverse it again right away. seems like unnecessary work. was that leftover by accident @BowenD-UCB?

@janosh janosh merged commit cde8a76 into CederGroupHub:main Sep 15, 2024
@janosh
Copy link
Copy Markdown
Collaborator

janosh commented Sep 15, 2024

@DanielYang59 given we're no longer reliant on materialyzeai/monty#712, chgnet should be ready for a new pypi release with numpy 2 support. anything i'm missing?

@DanielYang59
Copy link
Copy Markdown
Contributor Author

DanielYang59 commented Sep 15, 2024

chgnet should be ready for a new pypi release with numpy 2 support.

I believe we are :) Nothing missing AFAIK

@DanielYang59 DanielYang59 deleted the fix-monty-readline branch September 15, 2024 13:42
@DanielYang59 DanielYang59 changed the title Remove reverse readline, test again NP1 and recover NumPy 1 dependency support Remove reverse readline, test against NP1 and recover NumPy 1 dependency support Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@janosh janosh janosh approved these changes

Assignees

No one assigned

Labels

fix Bug fix PRs pkg Package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@DanielYang59 @janosh