-
Notifications
You must be signed in to change notification settings - Fork 14
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
add PseudoDojo #387
add PseudoDojo #387
Conversation
I think it would be worth to also include FR (though that will require that we add the SOC options for properties) and also to add the stringent accuracy |
Hi @AndresOrtegaGuerrero, Thanks for your suggestions. It's good to add more pseudo-family, but this leads to a longer time for the installation, thus the user needs to wait in the first run. The idea here is only to install the most commonly used pseudo, and trigger additional downloads (FR, stringent) if an advanced calculation is needed. This may be added in a further PR. See discussion #386. |
@superstar54 I agree, though it might be a good idea that the user gets some description that the PseudoDojo that they are using is SR, and that the accuracy is the standard |
Thanks!. A description for Pseudodojo has been added. |
Hi @superstar54, I change the base to branch Is there any block for this PR? Do we need more discussion on this? I remember we want to double-check the versioning of pseudo-dojo? |
I think in a previous meeting, we discussed as well that it could be useful to have normal and stringent precision |
Done |
@superstar54 Since the code was re factored, you should do a rebased on this branch with main , since now there is a folder named app for the files you modified |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #387 +/- ##
==========================================
+ Coverage 52.13% 52.63% +0.49%
==========================================
Files 16 17 +1
Lines 1849 1879 +30
==========================================
+ Hits 964 989 +25
- Misses 885 890 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
@AndresOrtegaGuerrero Thanks for the review. I addressed the issues and rebased the branch with the main branch. |
@superstar54 Thank you it looks nice, I tested and didn't observe any issues, I only have one opinion an is regarding the widget. |
Thanks for the suggestion. I updated the layout to show them in a 2x2 grid on a large screen and 4x1 on a small one. |
@superstar54 It looks great to me ! thank you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @superstar54, some change requests.
@unkcpz thanks for the review, I have addressed all the requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @superstar54, still have one thing want you to check. I approve it anyway so I do not block you for further progress.
BTW, do you think it is okay to add a test for checking using the other pseudofamily?
aiidalab_qe/app/pseudos.py
Outdated
if self.override_protocol_pseudo_family.value: | ||
if "PseudoDojo" in self.protocol_selection.value: | ||
pseudo_family = f"PseudoDojo/0.4/{self.dft_functional.value}/SR/{self.protocol_selection.value.split()[1]}/upf" | ||
elif "SSSP" in self.protocol_selection.value: | ||
pseudo_family = f"SSSP/1.2/{self.dft_functional.value}/{self.protocol_selection.value.split()[1]}" | ||
else: | ||
raise ValueError( | ||
f"Unknown pseudo family {self.override_protocol_pseudo_family.value}" | ||
) | ||
self.value = pseudo_family | ||
else: | ||
self.value = DEFAULT_PARAMETERS["pseudo_family"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit worried about the complex if..else nest here. Can you check it again if you can make it more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@superstar54 perhaps you can use a dictionary to put the options an evaluate the dict[value] ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndresOrtegaGuerrero, I don't quite understand what you mean. @superstar54, can you try to address it?
Then we can merge this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@superstar54 you can ignore my suggestion and merge if you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestions. I updated the code to make the string pretty and hopefully more clear. A test is also added for the PseudoFamilySelector
widget.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! All good to me.
Here, two pseudo families added:
I didn't touch the installation process, thus, it still uses the
aiida-pseudo install
method like that forSSSP
.GUI
The GUI of pseudo selection is now: