-
Notifications
You must be signed in to change notification settings - Fork 18
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 plugin: XPS #518
Add plugin: XPS #518
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #518 +/- ##
==========================================
- Coverage 77.48% 75.82% -1.67%
==========================================
Files 55 60 +5
Lines 4011 4277 +266
==========================================
+ Hits 3108 3243 +135
- Misses 903 1034 +131
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@PNOGillespie, Please review this PR. Thanks! |
ae6fab8
to
251399f
Compare
It seems that the plugins still use the default pseudo family from pw workchain.
|
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.
Hi @superstar54, I've had a look at the code and, generally speaking, it looks good. I've left some suggested changes here and there to improve readability and clean up commented-out blocks of code.
The only important change IMO is a selection widget for the element to display in the Result
panel - see the relevant comment for discussion.
voigt_profile_help = ipw.HTML( | ||
"""<div style="line-height: 140%; padding-top: 10px; padding-bottom: 10px"> | ||
The Voigt function: | ||
</div>""" | ||
) |
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.
Was there meant to be a description of the Voigt function here? I would suggest adding something for the user's benefit, at least just to tell them what the two slider values are.
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 added a description for the parameters and also a link to Wikipedia.
for element, data in spectra.items(): | ||
# print(element, data) | ||
for site, d in data.items(): | ||
g.add_scatter(x=d[0], y=d[1], fill="tozeroy", name=f"{element}_{site}") |
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.
Would it be possible to add a dropdown/selection widget to change between different elements? (e.g. like what the XAS plugin has)
On the one hand, the user should know that XPS spectra from different elements aren't comparable and to look at the different elements separately. On the other hand, it would tidy up the UI a lot - particularly when considering a structure with many inequivalent atoms (e.g. a complex molecule)
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! I added the selection dropdown for different elements, like in the XAS plugin.
|
||
pseudo_title = ipw.HTML( | ||
"""<div style="padding-top: 0px; padding-bottom: 0px"> | ||
<h4>Pseudopotential</h4></div>""" |
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.
Just to make things easier to read:
<h4>Pseudopotential</h4></div>""" | |
<h4>Core-Hole pseudopotential group</h4></div>""" |
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 changed as you suggested.
peak_title = ipw.HTML( | ||
"""<div style="padding-top: 0px; padding-bottom: 0px"> | ||
<h4>Select peak</h4></div>""" | ||
) | ||
peak_help = ipw.HTML( | ||
"""<div style="line-height: 140%; padding-top: 6px; padding-bottom: 0px"> | ||
The list of peaks to be considered for analysis. | ||
</div>""" | ||
) |
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.
Would something like this make sense for readability? "Core-Hole" or "Core-Level" seems like it would be more intuitive, but if "Peak" is the correct technical term in this case then don't worry.
peak_title = ipw.HTML( | |
"""<div style="padding-top: 0px; padding-bottom: 0px"> | |
<h4>Select peak</h4></div>""" | |
) | |
peak_help = ipw.HTML( | |
"""<div style="line-height: 140%; padding-top: 6px; padding-bottom: 0px"> | |
The list of peaks to be considered for analysis. | |
</div>""" | |
) | |
peak_title = ipw.HTML( | |
"""<div style="padding-top: 0px; padding-bottom: 0px"> | |
<h4>Select core-level</h4></div>""" | |
) | |
peak_help = ipw.HTML( | |
"""<div style="line-height: 140%; padding-top: 6px; padding-bottom: 0px"> | |
The list of core-levels to be considered for analysis. | |
</div>""" | |
) |
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! I change it to core-level
# self.core_hole_treatment_title, | ||
# self.core_hole_treatment_help, | ||
# self.core_hole_treatment, | ||
self.pseudo_title, | ||
self.pseudo_help, | ||
self.pseudo_group, | ||
self.peak_title, | ||
self.peak_help, | ||
ipw.HBox( | ||
[self.peak_list], | ||
), | ||
# self.supercell_title, | ||
# self.supercell_help, | ||
# ipw.HBox( | ||
# [self.supercell_min_parameter], | ||
# ), |
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.
There are quite a few commented-out sections here which should be cleaned up if possible:
# self.core_hole_treatment_title, | |
# self.core_hole_treatment_help, | |
# self.core_hole_treatment, | |
self.pseudo_title, | |
self.pseudo_help, | |
self.pseudo_group, | |
self.peak_title, | |
self.peak_help, | |
ipw.HBox( | |
[self.peak_list], | |
), | |
# self.supercell_title, | |
# self.supercell_help, | |
# ipw.HBox( | |
# [self.supercell_min_parameter], | |
# ), | |
self.pseudo_title, | |
self.pseudo_help, | |
self.pseudo_group, | |
self.peak_title, | |
self.peak_help, | |
ipw.HBox( | |
[self.peak_list], | |
), |
Does this also mean (based on the fact that supercell_min_parameter
is not used) that the supercell size used in calculation is defined entirely by the protocol?
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.
Yes. This would be more user friendly
bdcca63
to
85e777b
Compare
9f71bce
to
9268914
Compare
…alab-qe into feature/plugin/xps
55016fd
to
7953d87
Compare
b0c5f89
to
1ceec1b
Compare
@PNOGillespie Thanks for your review. I made the changes as you suggested. I added the dropdown button to select the spectrum for different elements. I also added a table to show the offset energies, which are used to calibrate the calculated binding energy. |
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.
Hi @superstar54. Code looks good for the most part. I've added a few final changes, after which I think this PR is complete.
g.add_scatter( | ||
x=d["x"], y=d["y"], fill=fill.value, name=d["site"] | ||
) |
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.
This section here causes a crash because you're calling the value from the UI element rather than converting it to a type of fill method (e.g. "tozeroy"):
g.add_scatter( | |
x=d["x"], y=d["y"], fill=fill.value, name=d["site"] | |
) | |
if fill.value: | |
fill_type = "tozeroy" | |
else: | |
fill_type = None | |
g.add_scatter( | |
x=d["x"], y=d["y"], fill=fill_type, name=d["site"] | |
) |
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! I changed it.
correction_energies_table = ipw.HTML( | ||
"""<h4>Offset Energies (δ)</h4> | ||
<div>The binding energies should be corrected by these constant offsets. These offsets mainly depend on chemical element and core level, and are determined by comparing the calculated core electron binding energy to the experimental one.</div> | ||
<table><tr><th>Core-level</th><th>Value (eV)</th></tr>""" | ||
) |
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.
The first sentence should remind the user how the correction energies are used in practice:
correction_energies_table = ipw.HTML( | |
"""<h4>Offset Energies (δ)</h4> | |
<div>The binding energies should be corrected by these constant offsets. These offsets mainly depend on chemical element and core level, and are determined by comparing the calculated core electron binding energy to the experimental one.</div> | |
<table><tr><th>Core-level</th><th>Value (eV)</th></tr>""" | |
) | |
correction_energies_table = ipw.HTML( | |
"""<h4>Offset Energies (δ)</h4> | |
<div>When comparing binding energies reported by AiiDALab-QE to experimental data, these should be corrected by the given offset listed below</div> | |
<table><tr><th>Core-level</th><th>Value (eV)</th></tr>""" | |
) |
I would also recommend moving the second sentence (or maybe the relevant concept or description) to the documentation since this is not really needed here.
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! I made the changes.
for core_level, value in correction_energies.items(): | ||
element = core_level.split("_")[0] | ||
if element not in self.spectrum_select_options: | ||
continue | ||
exp = value["exp"] | ||
correction_energies_table.value += ( | ||
f"<tr><td>{core_level}</td><td>{exp}</td></tr>" | ||
) | ||
return correction_energies_table |
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.
Just a little addition to make things clearer:
for core_level, value in correction_energies.items(): | |
element = core_level.split("_")[0] | |
if element not in self.spectrum_select_options: | |
continue | |
exp = value["exp"] | |
correction_energies_table.value += ( | |
f"<tr><td>{core_level}</td><td>{exp}</td></tr>" | |
) | |
return correction_energies_table | |
for core_level, value in correction_energies.items(): | |
element = core_level.split("_")[0] | |
if element not in self.spectrum_select_options: | |
continue | |
exp = value["exp"] | |
if exp >= 0.0: | |
sign = "+" | |
elif exp < 0: | |
sign = "" | |
correction_energies_table.value += ( | |
f"<tr><td>{core_level}</td><td>{sign}{exp}</td></tr>" | |
) | |
return correction_energies_table |
If the values can be positive or negative, then it's usually a good idea to give the sign explicitly even if the number is positive.
The string for the core level name should probably also have the underscore removed, since this doesn't format so well in non-monospace text. I understand this is also the way that the labels for the core levels are written in the Setting
panel, but I wouldn't worry about changing those at this stage.
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! The sign is indeed very important. I just noticed reverse the sign for all values.
Co-authored-by: Peter Gillespie <[email protected]>
deae7cd
to
9ce6b92
Compare
a6aae7b
to
8f8fd4a
Compare
@PNOGillespie, thanks for your review and very useful suggestions! I made all the changes as suggested. |
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.
Looks good to me, nice work @superstar54!
This PR adds a plugin for calculating X-ray Photoelectron Spectroscopy (XPS) spectra using the
XpsWorkChain
of aiida-quantumespresso package.Setting Panel
The Setting panel allows users to
Results Panel
The Result panel displays the final XPS spectrum.
Pseudopotentials
XPS calculations in QE require core-hole pseudopotentials. We supply a set of pseudopotentials specific to XPS calculations in the XPS Pseudopotential Repository. This repository provides the core-hole pseudopotentials as an AiiDA archive file. There are two kind of pseudopotentials available:
WorkChain
To make the setting as simple as possible for user. The following default setting are used.
core_hole_treatment
.full
for moleculexch_smear
for metalxch_fixed
for insulatorsupercell_min_parameter
updated based onprotocol
.