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

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

Merged
merged 7 commits into from
Sep 15, 2024

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Sep 15, 2024

Summary

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

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()

@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
Contributor Author

DanielYang59 commented Sep 15, 2024

@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 materialsvirtuallab/monty#712 (comment). I would expect reverse readers to be more RAM-efficient though (but still bring no benefit to the use case here).

Copy link
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
10 checks passed
@janosh
Copy link
Collaborator

janosh commented Sep 15, 2024

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

@DanielYang59
Copy link
Contributor Author

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
Labels
fix Bug fix PRs pkg Package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants