Bandswidget refactoring

[AndresOrtegaGuerrero]

In this PR, I am refactoring the BandsPdosWidget in order to only create the plot once , and apply updates using the helper in the model avoiding reploting.
There is also the inclusion of a widget to change the color of the traces

Closes #948

[codecov]

Codecov Report

Attention: Patch coverage is 50.83799% with 88 lines in your changes missing coverage. Please review.

Project coverage is 67.85%. Comparing base (ae520ea) to head (75373b0).
Report is 141 commits behind head on main.

Files with missing lines	Patch %	Lines
src/aiidalab_qe/common/bands_pdos/model.py	50.00%	34 Missing ⚠️
src/aiidalab_qe/common/bands_pdos/utils.py	31.70%	28 Missing ⚠️
...rc/aiidalab_qe/common/bands_pdos/bandpdosplotly.py	58.82%	14 Missing ⚠️
...rc/aiidalab_qe/common/bands_pdos/bandpdoswidget.py	66.66%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #941      +/-   ##
==========================================
- Coverage   68.38%   67.85%   -0.53%     
==========================================
  Files         110      110              
  Lines        6098     6206     +108     
==========================================
+ Hits         4170     4211      +41     
- Misses       1928     1995      +67     

Flag	Coverage Δ
python-3.11	67.85% <50.83%> (-0.53%)	⬇️
python-3.9	67.87% <50.83%> (-0.52%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[AndresOrtegaGuerrero]

[edan-bainglass]

Thanks @AndresOrtegaGuerrero. Huge improvement already!

A few comments:

1. The traces in the new color widget are showing
   in the label, e.g. Si-3s
   [0.0,0.0,0.0]. Can we dynamically replace it with @, so we get, for example, Si-3s @ [0.0 0.0 0.0]? TBD :)
2. Changing group or contribution is a bit jumpy, likely due to removing and adding traces. I think you can make this even smoother by simply updating the traces. Is this possible? Same goes for updating band projections.
3. This is very nice for bulk Si, but seems still too heavy for "real" use cases (the example you sent me). We need to think of further optimization, if possible.

I make no comments regarding the implementation w.r.t MVC at this time. Let's address the other parts first (if possible), then look at MVC adjustments if necessary.

[AndresOrtegaGuerrero]

Thanks @AndresOrtegaGuerrero. Huge improvement already!

A few comments:

1. The traces in the new color widget are showing in the label, e.g. Si-3s[0.0,0.0,0.0]. Can we dynamically replace it with @, so we get, for example, Si-3s @ [0.0 0.0 0.0]? TBD :)
2. Changing group or contribution is a bit jumpy, likely due to removing and adding traces. I think you can make this even smoother by simply updating the traces. Is this possible? Same goes for updating band projections.
3. This is very nice for bulk Si, but seems still too heavy for "real" use cases (the example you sent me). We need to think of further optimization, if possible.

I make no comments regarding the implementation w.r.t MVC at this time. Let's address the other parts first (if possible), then look at MVC adjustments if necessary.

Thanks @edan-bainglass

1. Giovanni Suggested to changed the atomic position to the index (But for the moment lets use @ and then in another PR I address the labels, based on the comments from Giovanni, what do you think? )

2)No, the traces can't be directly updated since the number of traces is different. But I have another commit i didnt push that kinda improves a bit the performance of getting the new data. (I avoided using threats [this might be the best solution but I can do it later once you explore the threats in the app?]) Let me push it and you test it.

MVC , I tried :D , but if you want I will address the first comments right now, and then you tell me the best way to modify this for proper MVC design ?

[edan-bainglass]

1. Giovanni Suggested to changed the atomic position to the index (But for the moment lets use @ and then in another PR I address the labels, based on the comments from Giovanni, what do you think? )

Hmm, index would be cleaner, but yes, let's just do @ for now.

2)No, the traces can't be directly updated since the number of traces is different.

I think for group and contribution, replacing the data is correct (remove, then add). For band projections, I think the number of traces should be the same, right? So you should be able to use update here?

But I have another commit i didnt push that kinda improves a bit the performance of getting the new data. (I avoided using threats [this might be the best solution but I can do it later once you explore the threats in the app?]) Let me push it and you test it.

I'll have a look 👍

MVC , I tried :D , but if you want I will address the first comments right now, and then you tell me the best way to modify this for proper MVC design ?

My point was that no comments will be made w.r.t MVC until the above is addressed. That's not to say that your implementation is bad 😅 Just that I won't be commenting on it yet. Note that we've been discussing internally more and more about MVC and how to best balance the standardization benefits with the development burden. TBD, so I wouldn't worry too much about the "perfect" MVC implementation at this stage 🙂

[AndresOrtegaGuerrero]

I think for group and contribution, replacing the data is correct (remove, then add). For band projections, I think the number of traces should be the same, right? So you should be able to use update here?

The number of traces of band projections also changes with the change of group and contribution.

My point was that no comments will be made w.r.t MVC until the above is addressed. That's not to say that your implementation is bad 😅 Just that I won't be commenting on it yet. Note that we've been discussing internally more and more about MVC and how to best balance the standardization benefits with the development burden. TBD, so I wouldn't worry too much about the "perfect" MVC implementation at this stage 🙂
Ok , just let me know , I am open to adapt (just help me a bit 😅)

BTW , the bottleneck it should be the get_data fuctions, and my changes didnt seem to help much , i will push it , and you let me know

[edan-bainglass]

I think for group and contribution, replacing the data is correct (remove, then add). For band projections, I think the number of traces should be the same, right? So you should be able to use update here?

The number of traces of band projections also changes with the change of group and contribution.

But the number of traces doesn't change w.r.t fat bands width, which currently ALSO removes and adds traces. My point is that in this case, it may be possible to simply update the existing data.

[AndresOrtegaGuerrero]

I think for group and contribution, replacing the data is correct (remove, then add). For band projections, I think the number of traces should be the same, right? So you should be able to use update here?

The number of traces of band projections also changes with the change of group and contribution.

But the number of traces doesn't change w.r.t fat bands width, which currently ALSO removes and adds traces. My point is that in this case, it may be possible to simply update the existing data.

Ah yes, in that case you are right. There is something in the logic , that I realize leads to a bug, lets say you have the fat bands and pdos (electronic tab) and let say that you put some numbers for a selection , you plot (click apply button), now after that lets say the user delete the selection but doesnt click "Apply selection" and instead goes and moves the slider of band fat , this will trigger that the PDOS is not updated since apply selection was not applied , but the fat bands projections will be different since it will not apply the selection. What do you suggest for this case ? ( to avoid it ) ?

[edan-bainglass]

I think for group and contribution, replacing the data is correct (remove, then add). For band projections, I think the number of traces should be the same, right? So you should be able to use update here?

The number of traces of band projections also changes with the change of group and contribution.

But the number of traces doesn't change w.r.t fat bands width, which currently ALSO removes and adds traces. My point is that in this case, it may be possible to simply update the existing data.

Ah yes, in that case you are right. There is something in the logic , that I realize leads to a bug, lets say you have the fat bands and pdos (electronic tab) and let say that you put some numbers for a selection , you plot (click apply button), now after that lets say the user delete the selection but doesnt click "Apply selection" and instead goes and moves the slider of band fat , this will trigger that the PDOS is not updated since apply selection was not applied , but the fat bands projections will be different since it will not apply the selection. What do you suggest for this case ? ( to avoid it ) ?

I followed your example without issue 🤷🏻‍♂️ I tried it on bulk Si and SiO2. I do see one issue (not sure if this is what you refer to) where if I select a specific atom to project on, then remove the selection but not click on apply, then finally update the projection, they are computed w.r.t no selection as if I did click apply. This is a bit confusing. I guess projections are computed w.r.t the current state of the widget and not the model? Or perhaps the textbox is linked with the model incorrectly.

[AndresOrtegaGuerrero]

for SiO2 , just put in the selection widget first 1..2 (and click apply selection), then deleted the selected atoms , and move the slider without click on apply selection. (use positions as group that will help you to see the issue) , the fat bands have different projection of all atoms , while the pdos will only keep for two atoms (since it isnt updated)

[edan-bainglass]

for SiO2 , just put in the selection widget first 1..2 (and click apply selection), then deleted the selected atoms , and move the slider without click on apply selection. (use positions as group that will help you to see the issue) , the fat bands have different projection of all atoms , while the pdos will only keep for two atoms (since it isnt updated)

Did you fix this in the recent push? Because it looks fine to me

[edan-bainglass]

Okay, the fat bands behavior is now quite nice. I'd love to re-enable continuous updates for this, but probably a bad idea w.r.t your crazy realistic systems 😅

Regarding the jumpy behavior when changing group/contribution, anything we can do there?

Also, can you add a space around @ in the labels, so "Si-3s @ [...]"? 🙏

Lastly, are you caching results? So for example, if I ask to group by atoms, you could compute it once and store it, so the next time I ask for this grouping, it doesn't need to recompute? Hopefully this is not too much to store in memory. Note that if you do this, atoms selection should not be cached, as I guess it simply indexes the existing data. Correct me if I'm wrong 🙂

[AndresOrtegaGuerrero]

Okay, the fat bands behavior is now quite nice. I'd love to re-enable continuous updates for this, but probably a bad idea w.r.t your crazy realistic systems 😅

I know (those systems are quite big xD ) , but at least is within 6 secs in my container :D , before it was minutes hahah ,

Regarding the jumpy behavior when changing group/contribution, anything we can do there?

Is because the space of the legend changes ,

Also, can you add a space around @ in the labels, so "Si-3s @ [...]"? 🙏

I will do this ,

Lastly, are you caching results? So for example, if I ask to group by atoms, you could compute it once and store it, so the next time I ask for this grouping, it doesn't need to recompute? Hopefully this is not too much to store in memory. Note that if you do this, atoms selection should not be cached, as I guess it simply indexes the existing data. Correct me if I'm wrong 🙂

In principle , I can cache them, though I was a lil concerned about memory ...
Bands are only computed once and stored.

Would you like that I cache the data (pdos, fat_bands) ? , (but only for cases that involved all atoms, no select atoms [that one we can afford to re-compute]), any suggestions for organizing the cached data? with a dictionary I guess and check if i have it there or not.

Regarding selected atoms, is difficult to know after is being grouped , unless you are thinking of something like

pdos_data = [
[{"kinds_orbital": {'data': data_n1 , 'selection': '' }}] ,
[{"kinds_atoms": {'data': data_n2, 'selection': ' [2,3,4,5]' }}
]
??

[AndresOrtegaGuerrero]

for SiO2 , just put in the selection widget first 1..2 (and click apply selection), then deleted the selected atoms , and move the slider without click on apply selection. (use positions as group that will help you to see the issue) , the fat bands have different projection of all atoms , while the pdos will only keep for two atoms (since it isnt updated)

Did you fix this in the recent push? Because it looks fine to me

No , that bug is still present.

[edan-bainglass]

for SiO2 , just put in the selection widget first 1..2 (and click apply selection), then deleted the selected atoms , and move the slider without click on apply selection. (use positions as group that will help you to see the issue) , the fat bands have different projection of all atoms , while the pdos will only keep for two atoms (since it isnt updated)

Did you fix this in the recent push? Because it looks fine to me

No , that bug is still present.

For future reference, we verified that the bug is indeed NO LONGER PRESENT 🎉 However, we also discussed that it would be good (important!) to add some notification to the user stating that a change in selection has not yet been applied and any results shown are w.r.t the previous selection. TBD

[edan-bainglass]

Regarding the jumpy behavior when changing group/contribution, anything we can do there?

Is because the space of the legend changes ,

This is annoying, but also not straight forward to resolve as far as I can see. I'll open an issue to be revisited!

Lastly, are you caching results? So for example, if I ask to group by atoms, you could compute it once and store it, so the next time I ask for this grouping, it doesn't need to recompute? Hopefully this is not too much to store in memory. Note that if you do this, atoms selection should not be cached, as I guess it simply indexes the existing data. Correct me if I'm wrong 🙂

In principle , I can cache them, though I was a lil concerned about memory ... Bands are only computed once and stored.

Would you like that I cache the data (pdos, fat_bands) ? , (but only for cases that involved all atoms, no select atoms [that one we can afford to re-compute]), any suggestions for organizing the cached data? with a dictionary I guess and check if i have it there or not.

Regarding selected atoms, is difficult to know after is being grouped , unless you are thinking of something like

pdos_data = [ [{"kinds_orbital": {'data': data_n1 , 'selection': '' }}] , [{"kinds_atoms": {'data': data_n2, 'selection': ' [2,3,4,5]' }} ] ??

Yeah, this whole matter of caching clearly requires further discussion. Let's revisit at a later PR.

[edan-bainglass]

Okay. LGTM! Let's get this in. We can discuss the near-future features after. Let me also state again that this is already a major step forward w.r.t bands/pdos plotting. Excellent work @AndresOrtegaGuerrero 💪🙏

[AndresOrtegaGuerrero]

Okay. LGTM! Let's get this in. We can discuss the near-future features after. Let me also state again that this is already a major step forward w.r.t bands/pdos plotting. Excellent work @AndresOrtegaGuerrero 💪🙏

Thank you @edan-bainglass also for the help and time !