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 for comment lines inside blocks #172

Closed

Conversation

pointedsphere
Copy link

Hi Jacob,

I'm Scott, one of Matt's PhD students that was looking at incorporating castep_outputs into some example scripts.

Found a small bug with the reading of a cell file where there are comments within a block that contain numbers. A bit of a niche thing, but the CASTEP output of final post geom-opt cells (with write_cell_structure) includes comments of this form.

This PR is to account for this possibility. Unfortunately cannot use re.split('[#!]',line)[0] to get the bit of the line without a comment as the end of the lines Kinetic eigenvalue # 1 = 0.19929544E+02 in CASTEP output files are also thrown away by this. As such, this just discards full comment lines.

Thanks,

Scott

@pointedsphere pointedsphere marked this pull request as draft September 30, 2024 14:46
@oerc0122 oerc0122 changed the title Fix for commnet lines inside blocks Fix for comment lines inside blocks Sep 30, 2024
@pointedsphere pointedsphere marked this pull request as ready for review September 30, 2024 14:49
@oerc0122
Copy link
Owner

oerc0122 commented Sep 30, 2024

This seems like a far-reaching and potentially harmful change to automatically ignore any/all #/! characters in any file parsed by castep_outputs.

I would suggest instead a separate strip_comments filter in the utilities.

comment_re = re.compile(r"^\s*#")
itertools.filterfalse(comment_re.match, my_data)

@pointedsphere
Copy link
Author

I agree it is a bit drastic.

I'm not 100% on exactly how you were suggesting to add the filter. I've modified the PR to use itertools.filterfalse in the block reading to ignore full line comments. However, if I've missed the mark very happy to close the commit and leave it as an issue in the tracker.

@@ -203,7 +204,7 @@ def from_re(
data.append(init_line)

found = 0
for line in in_file:
for line in filterfalse(COMMENT_LINE_RE.match,in_file):
Copy link
Owner

Choose a reason for hiding this comment

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

Like I said, I don't think this should be applied generally in FileWrapper, but as a utility function when parsing chunks of data where comments should be stripped.

@oerc0122
Copy link
Owner

oerc0122 commented Oct 1, 2024

I agree it is a bit drastic.

I'm not 100% on exactly how you were suggesting to add the filter. I've modified the PR to use itertools.filterfalse in the block reading to ignore full line comments. However, if I've missed the mark very happy to close the commit and leave it as an issue in the tracker.

@pointedsphere c.f. #173 and check that meets your needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants