New BandsPdosWidget #581
New BandsPdosWidget #581
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #581 +/- ##
==========================================
+ Coverage 77.55% 80.63% +3.08%
==========================================
Files 54 50 -4
Lines 3831 3609 -222
==========================================
- Hits 2971 2910 -61
+ Misses 860 699 -161
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Please have a look , test it let me know any further changes and/or developments @unkcpz @superstar54 . |
|
I think in a second PR, i can add different test for the new widget |
|
Pinning @mikibonacci, he mentioned he has a use case that needs some adaption on the bandsplotwidget which will be replaced by this PR, so he can give a quick review and a test. I didn't start looking at the code, but give it a try on the qeapp, from the outcome it is exactly what we needed, thanks for the great work @AndresOrtegaGuerrero. |
Thank you! , please let me know we can gather ideas of what is missing ,or a different approach , or for the future, |
There was a problem hiding this comment.
@AndresOrtegaGuerrero I give it a first go.
I notice that the data validation is missing. The BandPdosWidget accept AiiDA node as input which make the widget not generic and vulnerable to the future change of BandData.
In the original bandsplotwidget, we add a middle layer of dict type between AiiDA node and input for the widget. The object pass to the widget then can be validated by the schema.
I'd suggest adding this design back to the new widget. For the data validation, you can try to use pydantic.
Besides, the unit tests for the newly added widget would be very useful.
@unkcpz Thank you , you mean that the widget doesnt call |
There was a problem hiding this comment.
Thanks for the changes @AndresOrtegaGuerrero. I have some request upon method names and especially the flow of data/objects.
This is a widget for dealing with the bands/pdos data, convert the data to the format that plotly can plot a plot object. Then generate the plotly plot object and render it to the ipyoutput.
I'd suggest at least separate methods for plotly plot object creating to a class BandPdosPlot. The class getting a formatted bands/pdos data and knowing how to create a plotly plot object. Then for the BandPdosWidget, it dealing with 1) parsing the data to the formatted data and pass to BandPdosPlot 2) dealing the logic with the QeApp, such as initialize the widget and hide the widget.
| - description: HTML description of the widget. | ||
| - dos_atoms_group: Dropdown widget to select the grouping of atoms for PDOS plotting. | ||
| - dos_plot_group: Dropdown widget to select the type of PDOS contributions to plot. | ||
| - selected_atoms: Text widget to select specific atoms for PDOS plotting. | ||
| - wrong_syntax: HTML widget to display an error message for incorrect atom selection syntax. | ||
| - update_plot_button: Button widget to update the plot. | ||
| - download_button: Button widget to download the data. | ||
| - export_dir: Path to the directory for exporting data. | ||
| - dos_data: PDOS data. | ||
| - fermi_energy: Fermi energy value. | ||
| - bands_data: Band structure data. | ||
| - band_labels: Labels and values for band structure plotting. | ||
| - bands_xaxis: X-axis settings for band structure plot. | ||
| - bands_yaxis: Y-axis settings for band structure plot. | ||
| - dos_xaxis: X-axis settings for PDOS plot. | ||
| - dos_yaxis: Y-axis settings for PDOS plot. | ||
| - bandsplot_widget: Plotly widget for band structure and PDOS plot. | ||
| - bands_widget: Output widget to display the bandsplot widget. | ||
| - pdos_options_out: Output widget to clear specific widgets. |
There was a problem hiding this comment.
I think most of these attributes don't need to be exposed.
|
|
||
| def _band_xaxis(self): | ||
| """Function to return the xaxis for the bands plot.""" | ||
| import plotly.graph_objects as go |
There was a problem hiding this comment.
Any reason to put the import for each function? If not, please move it to the top.
| if my_clean_labels: | ||
| if i not in my_clean_labels: | ||
| if my_clean_labels[-1][-1] == i[1]: |
There was a problem hiding this comment.
| if my_clean_labels: | |
| if i not in my_clean_labels: | |
| if my_clean_labels[-1][-1] == i[1]: | |
| if my_clean_labels and (i not in my_clean_labels) and (my_clean_labels[-1][-1] == i[1]): |
I guess this is the same? If not can you try to simplify it, too many layers of nest indicate things that can be polished.
| # Output widget to clear the specific widgets | ||
| self.pdos_options_out = ipw.Output() | ||
|
|
||
| def download_data(_=None): |
There was a problem hiding this comment.
When i do it , it doesnt work i will check if i can move it out
| # Information for the plot | ||
| self.dos_data = self._get_dos_data() | ||
| self.fermi_energy = self._get_fermi_energy() | ||
| self.bands_data = self._get_bands_data() | ||
| self.band_labels = self._bands_labeling(self.bands_data) |
There was a problem hiding this comment.
I didn't see the reason why these attributes need to initialize here. When the widget initialized, there is no plot, so these attributes are not needed.
|
|
||
| return dosyaxis | ||
|
|
||
| def _bandsplot_widget(self): |
There was a problem hiding this comment.
The method name is bandsplot but it also deal with pdos, better to be called as bandpdosplot.
Moreover, this function return a plotly plot objcet, so I'd call it get_bandpdos_plot.
The data flow is more clean to be like: data -> plotly plot object -> ipyoutput widget, can you check the PR and try to apply this flow?
|
Hi @AndresOrtegaGuerrero, it is all in good shape. Before merge, I want to ask for a final test on using the new widget for the phonon bands plot. Can you or @mikibonacci give a test on using the new widget for it? I think it is a good proof that the API exposed by the new widget is generic enough for most of use cases. |
|
Since this replace the |
The old bandsplot widget https://github.com/aiidalab/widget-bandsplot was separately maintained in the repo with complex and not well modulized JS code. In this PR, the new bandspdoswidget is introduced to have it all implemented in python and in ipywidget framework with using plotly as plot engine. The output band structure is in a publish ready level with a flexible control on the groups of orbitals/atoms user want to projected.
de6723d to
353a3c3
Compare
This PR introduces a new bands_pdos_widget using plotly, the widget include options of drag, pan and toogle , save a snapshot , and download the data.
The formating of the bands/pdos has been moved from the result plugin to the common/bandspdoswidget.py
It allows different selection of atoms to consider, or different representations ( We can modified or create new according to your suggestions)
For spin polarized calculations it represents bands_up in red, and bands_down in blue. For non polarized the line is in black color.
The new
_projections_curated_optionsallows to curated SOC pdos.The legend has a wheel fixing #552