-
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
Bandswidget bump to 0.5.0, make the data format compatible #313
Conversation
e14c532
to
50effca
Compare
@unkcpz could you add a short description for this PR? What's the purpose and what has been added? |
Hi @superstar54, I'd say all the tangible changes are made from bandsplot widget, here in QeApp the change is made to compatible with the new API to plot the bands and dos. |
Combine mig2.x small port fix setup code posix conde codes Add test Test dep split [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Restore qe ipynb widget bands plot bumpver Add _projections_curated funcion for data parser
for more information, see https://pre-commit.ci
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 got this error for "SiO2" + "fast"
---------------------------------------------------------------------------
KeyError Traceback (most recent call last)
~/apps/aiidalab-qe/aiidalab_qe/widgets.py in _observe_node(self, change)
382 clear_output()
383 if change["new"]:
--> 384 display(viewer(change["new"]))
385
386
~/apps/aiidalab-widgets-base/aiidalab_widgets_base/viewers.py in viewer(obj, downloadable, **kwargs)
81 )
82 return obj
---> 83 raise exc
84
85
~/apps/aiidalab-widgets-base/aiidalab_widgets_base/viewers.py in viewer(obj, downloadable, **kwargs)
73 try:
74 _viewer = AIIDA_VIEWER_MAPPING[obj.node_type]
---> 75 return _viewer(obj, downloadable=downloadable, **kwargs)
76 except (KeyError) as exc:
77 if obj.node_type in str(exc):
~/apps/aiidalab-qe/aiidalab_qe/node_view.py in __init__(self, node, **kwargs)
559
560 self.result_tabs.observe(on_selected_index_change, "selected_index")
--> 561 self._update_view()
562
563 super().__init__(
~/apps/aiidalab-qe/aiidalab_qe/node_view.py in _update_view(self)
586 "band_structure" in self.node.outputs or "dos" in self.node.outputs
587 ):
--> 588 self._show_electronic_structure()
589 self._results_shown.add("electronic_structure")
590
~/apps/aiidalab-qe/aiidalab_qe/node_view.py in _show_electronic_structure(self)
600 bands_data = data.get("bands", None)
601 dos_data = data.get("dos", None)
--> 602 self._bands_plot_view = BandsPlotWidget(
603 bands=bands_data, dos=dos_data, plot_fermilevel=True
604 )
/opt/conda/lib/python3.9/site-packages/widget_bandsplot/bandsplot.py in __init__(self, bands, dos, fermi_energy, show_legend, plot_fermilevel, energy_range)
72 if dos is not None:
73 temp_dos = deepcopy(dos)
---> 74 self.tdos_x = dos['tdos']['energy | eV']['data']
75 self.tdos_y = dos['tdos']['values']['dos | states/eV']['data']
76 self.dos_fermienergy = dos['fermi_energy']
KeyError: 'tdos'
I am not testing "SiO2" + "relax" + "bands" + "pdos" + "moderate", will let you know when the test finishs.
if curated_tag == "atom": | ||
curated_tag_var = atom_position | ||
else: | ||
# by orbital label | ||
curated_tag_var = orbital_name |
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.
Adding positions to the label is not common and makes the plot too much test. I suggest using the index of the atoms.
Previously, the orbital label is used. I think we can keep using the orbital label.
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 problem of having orbitals distinguished is will having many orbitals if having elements with d-orbital included.
It has 1s, 3p, 5d for every element. I don't think there is a default
way to properly show this. To me, the index of the atom has little meaning.
Let's keep this open issue and discuss it further with other people (especially the demo testing with Nicola).
We can either keep this open, or merge this and show the non-perfect result to people and get feedback.
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 really want to keep this thoroughly discussed and find and good solution on our side, however, the demo with Nicola really push us to have a "non-perfect" version. Sorry for the rush, I am not reluctant to accept your suggestion. Overall, this is to make the newly refactored bandswidget
work, there are plenty of things to improve after, but we can leave it to the near future.
@superstar54 thanks for testing! For the error, can you reinstall the package with |
Hi @superstar54, I'll merge this now so I can move on to test. If new things pop up we can tackle it then. |
fixes #229