-
Notifications
You must be signed in to change notification settings - Fork 16
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
DOC: add xps to howto #438
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #438 +/- ##
==========================================
- Coverage 75.83% 75.79% -0.05%
==========================================
Files 60 60
Lines 4295 4300 +5
==========================================
+ Hits 3257 3259 +2
- Misses 1038 1041 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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, good work putting together a simple documentation set.
I've added some suggested some minor changes to fix typos and grammar in some places.
The more substantive changes are:
- Step 2 is missing a picture of the XPS Settings tab.
- There are parts towards the end which talk about the
offset energies
and the spin-orbit splitting, but lacks a prior step which explains how to export the data (beyond reading the value from the Plotly widget) - The real world example of ETFA doesn't seem to give much useful information for the user about the plugin (see the relevant comment for more information).
docs/source/howto/xps.rst
Outdated
When comparing the calculated binding energies to the experimental data, please correct the energies by the given offset listed in the ``Offset Energies`` table. | ||
Besides, the DFT calculated binding energies do not include spin-orbit splitting of the core level state. | ||
We can include the spin-orbit splitting using its experimental value. | ||
Take `f` orbit as a example, we need subtracting :math:`3/7` of the experimental spin-orbit splitting or adding :math:`4/7` of the DFT calculated value, to get the position of the :math:`4f_{7/2}` and :math:`4f_{5/2}` peaks, respectively. |
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 could be useful, but (unless I'm mistaken) there is no way to extract the exact binding energy values from AiiDALab-QE.
If there is a way to do this, then please update some part of the text with instructions on how to do so, otherwise this doesn't provide much utility and should be removed for simplicity.
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 pointing out this. I add the detail text to tell user how to find the exact binding energies.
docs/source/howto/xps.rst
Outdated
A real world example | ||
==================== | ||
|
||
Here is a XPS calculation for ETFA molecule, which is also referred to as the “ESCA molecule” in the literature. [1] | ||
|
||
.. figure:: /_static/images/xps_etfa_dft.png | ||
:align: center | ||
|
||
One can compared the calculated chemical shift with the experimental data. | ||
|
||
.. figure:: /_static/images/xps_etfa_exp.jpg | ||
:align: center | ||
|
||
|
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 last example doesn't seem to provide much for the user's benefit, for a few reasons:
- The calculated and experimental spectra aren't plotted together for comparison.
- Without a set of instructions, or source for the structure, the user can't use this like some sort of complex "real-world" example as an extension of the documentation and so it doesn't really add much.
I would suggest either removing the section or replacing the example with one for Phenacetylene, which we have tested already and can provide figures/data for.
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 added the ETFA molecule into the example so users can use it. also, add a tip to tell users how to run the calculation.
Currently, the app does not provide the functionality to add experimental data to the plot. But one can also compare the two plots together. I don't plot them manually because I don't want to give the user the impression that this can be done in the app directly.
Instead of giving an example that the calculated values match the experiment very well, the real
example here shows the difference between calculated values and the experimental values. This is important that, the user knows our app is powerful, but there are still some limitations due to the underlying method.
Co-authored-by: Peter Gillespie <55498719+PNOGillespie@users.noreply.github.com>
Co-authored-by: Peter Gillespie <55498719+PNOGillespie@users.noreply.github.com>
Co-authored-by: Peter Gillespie <55498719+PNOGillespie@users.noreply.github.com>
Co-authored-by: Peter Gillespie <55498719+PNOGillespie@users.noreply.github.com>
Co-authored-by: Peter Gillespie <55498719+PNOGillespie@users.noreply.github.com>
@PNOGillespie thanks for the review. I revised the page based on your suggestions.
I added a picture.
I added a detailed instruction.
I replied after your comment. Please have a second review. |
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. Looks good for the most part, particularly in providing a contrast between ETFA and Phenylacetylene. Just a few proofreading changes to make, then I think this is good to go.
Since you have added a spectrum upload tool (which seems to work fine, from what i can see) I would suggest (in a different PR) one improvement, which would be to add a UI element to scale the intensities of the computed XPS traces so as to make it easier to compare the profile of (in particular) the total XPS to the experimental data.
Again, I would just suggest you open an improvements
issue for that and address that at a later time.
Co-authored-by: Peter Gillespie <55498719+PNOGillespie@users.noreply.github.com>
Co-authored-by: Peter Gillespie <55498719+PNOGillespie@users.noreply.github.com>
Co-authored-by: Peter Gillespie <55498719+PNOGillespie@users.noreply.github.com>
Co-authored-by: Peter Gillespie <55498719+PNOGillespie@users.noreply.github.com>
Co-authored-by: Peter Gillespie <55498719+PNOGillespie@users.noreply.github.com>
Co-authored-by: Peter Gillespie <55498719+PNOGillespie@users.noreply.github.com>
Thanks @PNOGillespie . I made the changes and also created an issue as you suggested. #627 |
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.
Good to go. Nice work @superstar54! Thanks also for adding the issue page.
This PR adds the doc for XPS calculation using the
plugin_workchain
branch of aiidalab-qe.I also improved the XPS plugin by: