aiidalab / aiidalab-qe

AiiDAlab App for Quantum ESPRESSO
https://aiidalab-qe.readthedocs.io/
MIT License
12 stars 14 forks source link

Layout axes remain the same after update_plot methods is called #904

Closed AndresOrtegaGuerrero closed 3 weeks ago

AndresOrtegaGuerrero commented 4 weeks ago

This address this issue #903

codecov[bot] commented 4 weeks ago

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 67.09%. Comparing base (15f46b0) to head (7d1a91b). Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/aiidalab_qe/common/bandpdoswidget.py 0.00% 6 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #904 +/- ## ========================================== - Coverage 67.15% 67.09% -0.06% ========================================== Files 51 51 Lines 4707 4711 +4 ========================================== Hits 3161 3161 - Misses 1546 1550 +4 ``` | [Flag](https://app.codecov.io/gh/aiidalab/aiidalab-qe/pull/904/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | Coverage Δ | | |---|---|---| | [python-3.11](https://app.codecov.io/gh/aiidalab/aiidalab-qe/pull/904/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `?` | | | [python-3.9](https://app.codecov.io/gh/aiidalab/aiidalab-qe/pull/904/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `67.09% <0.00%> (-0.09%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

superstar54 commented 3 weeks ago

Hi @AndresOrtegaGuerrero , should we reset the axis range every time the plot updates? Maybe in some cases, when we change the plot setting, the axis range should be updated.

Another issue (not related to this PR), when I change the flat parameters, the screen flash a lot:

https://github.com/user-attachments/assets/a5909271-2f4a-4623-9fac-677e138702e7

In the flat band case, is it possible to update the data instead of re-drawing the plot?

AndresOrtegaGuerrero commented 3 weeks ago

Hi @AndresOrtegaGuerrero , should we reset the axis range every time the plot updates? Maybe in some cases, when we change the plot setting, the axis range should be updated.

Another issue (not related to this PR), when I change the flat parameters, the screen flash a lot:

qeapp-flat-bands-falsh.mp4 In the flat band case, is it possible to update the data instead of re-drawing the plot?

To answer the first question , we have the reset axis button from plotly that does this function.

The second question , we do a replot since we have to widget a BandsPdosWidget (meant for qe app) and the BandsPdosPlotly , that kinda resemble the bandsplot-widget from the past that just receives a dictionary with the info.

For a new thinckness i need to re-loop and re compute contribution per band.

For the flashing , i think is that each time we mode the widget it is triggering the replot .... maybe i could add like a time to react ?

superstar54 commented 3 weeks ago

For a new thinckness i need to re-loop and re compute contribution per band.

If the number of data is the same, then it is possible to only update the data instead of re-plot the image. I did this in the XPS plot, as shown here: https://github.com/aiidalab/aiidalab-qe/blob/15f46b03e4b1367c6270b6f0db71068960900c06/src/aiidalab_qe/plugins/xps/result.py#L219

You can look at it in the future when you have time.

AndresOrtegaGuerrero commented 3 weeks ago

@superstar54 Thank you! , regarding the flashing , what about adding like a debounce of 0.5 sec (or less) ?

Regarding the updating the plot , so i will try to find a way to avoid repeating a re-loops. Maybe in a different PR ?

superstar54 commented 3 weeks ago

Hi @AndresOrtegaGuerrero , yes, Okay to merge the PR. You can implement the update in a future PR when you have time.