search

oerc0122 / castep_outputs

Parser for CASTEP output files
BSD 3-Clause "New" or "Revised" License
2 stars 3 forks source link

added functionality for reading .phonon files #133

Closed theloanerkit closed 3 months ago

theloanerkit commented 3 months ago

I've added a parser for .phonon files. I needed to parse .phonon files for a project I am currently working on.

theloanerkit commented 3 months ago

Hi, I (think) I have added the changes you asked for. I had a little trouble getting gen_data.py to run.

Best wishes, Kit

On Tue, Jul 23, 2024 at 10:20 AM Jacob Wilkins @.***> wrote:

@.**** requested changes on this pull request.

Thanks for this, but it needs tests before it can go in. To do so:

  • Take a small, but representative example of a phonon file, and place it in the tests folder as test.phonon
  • Add "phonon" to gen_data.py, run it and check the resulting phonon.json and phonon.yml files (N.B. due to different library versions doing sorted/unsorted dumps this may change some other files, which you may want to reset)
  • Add a test to test/test_dumpers.py (apologies, I still need to clean that up as a test factory)

In castep_outputs/parsers/phonon_file_parser.py https://github.com/oerc0122/castep_outputs/pull/133#discussion_r1687704618 :

  • eigenvectors_endblock = ""
  • eigenvectors_endblock += " "*(4-len(str(phonon_info["branches"])))+str(phonon_info["branches"])
  • eigenvectors_endblock += " "*(4-len(str(phonon_info["ions"])))+str(phonon_info["ions"])

Probably simpler would be: ⬇️ Suggested change

  • eigenvectors_endblock = ""
  • eigenvectors_endblock += " "*(4-len(str(phonon_info["branches"])))+str(phonon_info["branches"])
  • eigenvectors_endblock += " "*(4-len(str(phonon_info["ions"])))+str(phonon_info["ions"])
  • eigenvectors_endblock = (format(phonon_info["branches"], ">4") +
  • format(phonon_info["ions"], ">4"))

In castep_outputs/parsers/phonon_file_parser.py https://github.com/oerc0122/castep_outputs/pull/133#discussion_r1687705357 :

  • phonon_info["qpt_pos"] = []
  • phonon_info["evals"] = []
  • phonon_info["evecs"] = []

Given these default to lists, is it important to assign them here?

In castep_outputs/parsers/phonon_file_parser.py https://github.com/oerc0122/castep_outputs/pull/133#discussion_r1687706265 :

  • evals = []
  • evecs = []
  • for line in phonon_file:
  • if block := get_block(line, phonon_file, "BEGIN header", "END header"):
  • data = parse_regular_header(block)
  • phonon_info.update(data)
  • eigenvectors_endblock = ""
  • eigenvectors_endblock += " "*(4-len(str(phonon_info["branches"])))+str(phonon_info["branches"])
  • eigenvectors_endblock += " "*(4-len(str(phonon_info["ions"])))+str(phonon_info["ions"])
  • elif block := get_block(line, phonon_file, "q-pt", "Phonon Eigenvectors", out_fmt=list):
  • logger("Found eigenvalue block")
  • b = []

b seems unused.

In castep_outputs/parsers/phonon_file_parser.py https://github.com/oerc0122/castep_outputs/pull/133#discussion_r1687718981 :

  • e_vec = []
  • for i in range(0,len(vectors), 2):
  • e_vec.append([qdata['evec'][i],qdata['evec'][i+1]])
  • qdata['evec'] = e_vec

⬇️ Suggested change

  • e_vec = []
  • for i in range(0,len(vectors), 2):
  • e_vec.append([qdata['evec'][i],qdata['evec'][i+1]])
  • qdata['evec'] = e_vec
  • qdata['evec'] = [[qdata['evec'][i], qdata['evec'][i+1]]
  • for i in range(0,len(vectors), 2)]

Although it would also be possible to do: ⬇️ Suggested change

  • e_vec = []
  • for i in range(0,len(vectors), 2):
  • e_vec.append([qdata['evec'][i],qdata['evec'][i+1]])
  • qdata['evec'] = e_vec
  • qdata['evec'] = list(zip(qdata['evec'][::2], qdata['evec'][1::2]))

— Reply to this email directly, view it on GitHub https://github.com/oerc0122/castep_outputs/pull/133#pullrequestreview-2193408447, or unsubscribe https://github.com/notifications/unsubscribe-auth/A6BZHOUMOBX3SBS3BUUMCKLZNYN4JAVCNFSM6AAAAABLJ6HYYKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOJTGQYDQNBUG4 . You are receiving this because you authored the thread.Message ID: @.***>

oerc0122 commented 3 months ago

Hi, I (think) I have added the changes you asked for. I had a little trouble getting gen_data.py to run. - I had to change from castep_outputs.castep_outputs_main import parse_all to from castep_outputs.cli.castep_outputs_main import parse_all in gen_data.py - I also had to add the full path for it to find and output the files correctly, although this is probably a my system problem Best wishes, Kit

Nope, that's my error. Thanks for catching it. Recently had a major refactor of the code and it got left out in the fixes.

oerc0122 commented 3 months ago

This looks to be a version issue with ruamel.yaml, I'll try to sort it in another PR soon.