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

added pdos job and flow #1643

Closed
wants to merge 3 commits into from

Conversation

Nekkrad
Copy link
Contributor

@Nekkrad Nekkrad commented Jan 31, 2024

Summary of Changes

Implemented a pdos job and a pdos flow (use of projwfc.x binary).

There are multiple files created and in theory I could create custom functions to read all the relevant information and store them in the result dictionary. Another option is to read the xml file via either a big custom function or the use of xml_to_dict python package but that means we have to add a new dependency.

Requirements

Checklist

  • I have read the "Guidelines" section of the contributing guide. Don't lie! 😉
  • My PR is on a custom branch and is not named main.
  • I have used black, isort, and ruff as described in the style guide.
  • I have added relevant, comprehensive unit tests.

@buildbot-princeton
Copy link
Collaborator

Can one of the admins verify this patch?

@Nekkrad
Copy link
Contributor Author

Nekkrad commented Jan 31, 2024

  1. It seems that projwfc.x has the same problem as dos.x which causes the test to fail. I have more recipes ready for the other binaries (bands.x and fermi surface). Should I still open new PRs or should we try and see if we can get the binaries to work

  2. Puzzled about the DeepSource problem. I understand what it says but it works fine. Should I ignore it?

@Andrew-S-Rosen
Copy link
Member

Andrew-S-Rosen commented Jan 31, 2024

New dependencies are okay because we can make them optional (#1614). That said, my preference would be for dependencies that are actively maintained (xml_to_dict hasn't had a commit in 10 months).

No worries about the deepsource issue. It's a bug I've reported to their team. I'll disable that error message over the repo later.

Regarding the binary, that's odd. I personally don't want to merge in code without passing tests, so we will probably need to either: figure out how to solve it, work around it, or (if we absolutely must) mock the Espresso call.

@Nekkrad
Copy link
Contributor Author

Nekkrad commented Jan 31, 2024

Regarding the binary, that's odd. I personally don't want to merge in code without passing tests, so we will probably need to either: figure out how to solve it, work around it, or (if we absolutely must) mock the Espresso call.

So the error seems to be related to some missing library (as in dos.x ) . I will try to conda install qe instead of my compiled one to see what happens.

@tomdemeyere
Copy link
Contributor

tomdemeyere commented Jan 31, 2024

Can you give here the result of ldd $(which dos.x) ?

@Nekkrad
Copy link
Contributor Author

Nekkrad commented Feb 5, 2024

Can you give here the result of `ldd $(which dos.x)?

I think is asking you @Andrew-S-Rosen

@Nekkrad Nekkrad closed this Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants