Skip to content
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

Reset GUI #489

Merged
merged 15 commits into from
Oct 15, 2023
Merged

Reset GUI #489

merged 15 commits into from
Oct 15, 2023

Conversation

superstar54
Copy link
Member

@superstar54 superstar54 commented Sep 26, 2023

Reset the GUI when selecting to start a new workflow. Related to #437 #420.

Reset:

  • step 1, Structure (structure and viewer)
  • step 2, Configuration (all panels)
  • step 3, computational resource. (code and resources)

Step 4 will be automatically reset when the process of step 3 is reset.

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (967c3f8) 77.29% compared to head (fbee479) 78.29%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #489      +/-   ##
==========================================
+ Coverage   77.29%   78.29%   +0.99%     
==========================================
  Files          44       44              
  Lines        3127     3160      +33     
==========================================
+ Hits         2417     2474      +57     
+ Misses        710      686      -24     
Flag Coverage Δ
python-3.10 78.29% <100.00%> (+0.99%) ⬆️
python-3.8 78.33% <100.00%> (+0.99%) ⬆️
python-3.9 78.33% <100.00%> (+0.99%) ⬆️

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

Files Coverage Δ
src/aiidalab_qe/app/configuration/__init__.py 98.75% <100.00%> (+2.50%) ⬆️
src/aiidalab_qe/app/configuration/advanced.py 98.13% <100.00%> (+3.90%) ⬆️
src/aiidalab_qe/app/configuration/pseudos.py 88.79% <100.00%> (+0.14%) ⬆️
src/aiidalab_qe/app/configuration/workflow.py 100.00% <100.00%> (ø)
src/aiidalab_qe/app/main.py 96.42% <100.00%> (+6.59%) ⬆️
src/aiidalab_qe/app/structure/__init__.py 98.38% <100.00%> (+8.55%) ⬆️
src/aiidalab_qe/app/submission/__init__.py 77.44% <100.00%> (+1.91%) ⬆️
src/aiidalab_qe/plugins/bands/setting.py 100.00% <100.00%> (ø)
src/aiidalab_qe/plugins/pdos/setting.py 97.36% <100.00%> (+0.14%) ⬆️
tests/conftest.py 96.26% <100.00%> (+0.01%) ⬆️
... and 2 more

... and 1 file with indirect coverage changes

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

@unkcpz unkcpz added this to the v2023.10.0 milestone Oct 4, 2023
@AndresOrtegaGuerrero
Copy link
Member

@superstar54 I did a test, and it seems the Smearing type: , Smearing width (Ry) arent reset , also Wavefunction cutoff (Ry)
Charge density cutoff (Ry) . I am also wondering if the options in the panels get also reset ? I guess each panel needs a method named reset ?

@superstar54
Copy link
Member Author

superstar54 commented Oct 9, 2023

@AndresOrtegaGuerrero Thanks for the review.

it seems the Smearing type: , Smearing width (Ry) arent reset , also Wavefunction cutoff (Ry)
Charge density cutoff (Ry) .

I fixed it. Also fixed the reset in pseudo_setter and magnetization.

I am also wondering if the options in the panels get also reset ? I guess each panel needs a method named reset ?

yes, each panel needs to implement a reset method, which will be called from the ConfigureQeAppWorkChainStep.

@AndresOrtegaGuerrero
Copy link
Member

@superstar54 Would it be necessary to add in the documentation of qe_app in create your plugin , to include a reset method, to avoid future conflicts ?

@AndresOrtegaGuerrero
Copy link
Member

AndresOrtegaGuerrero commented Oct 10, 2023

@superstar54 I tested the PR, now it does update the fields i told you, the one missing is the one from the nscf kpoints distance from PDOS panel. I also noticed some behavior. I started the app and select one workchain and then another , and then the first one again. (Everything was working) after another change (the previous workchain) the app crashed.
Screenshot 2023-10-10 at 09 47 23

@AndresOrtegaGuerrero
Copy link
Member

@superstar54 another thing i noticed is that one of my workchain , (newer one) doesnt set the input parameters, but the another one i was using for testing it does. I know this is related to Load GUI PR.

@superstar54
Copy link
Member Author

Would it be necessary to add in the documentation of qe_app in create your plugin , to include a reset method, to avoid future conflicts ?

Good point. I have added it.

the one missing is the one from the nscf kpoints distance from PDOS panel.

After resetting, the pdos panel is not visible anymore. how do you check this?

I also noticed some behavior. I started the app and select one workchain and then another , and then the first one again.

This is because I set the structure of the structure_step two times in different places. I have fixed it. Please test again.

another thing i noticed is that one of my workchain , (newer one) doesnt set the input parameters, but the another one i was using for testing it does.

Which workchain? In my test, the load_GUI is OK.

@AndresOrtegaGuerrero
Copy link
Member

Which workchain? In my test, the load_GUI is OK.

It seems my workchain is "old" so i tried with a new one an no issues , maybe that workchain is incompatible with the load_GUI, but indeed i tried a new one and everything upload correctly

@@ -278,7 +279,7 @@ def _on_cutoff_change(self, _=None):
self.ecutrho = self.ecutrho_setter.value

def _reset_cutoff_widgets(self):
self.ecutrho_setter.value = 0
self.ecutwfc_setter.value = 0
self.ecutrho_setter.value = 0

def _reset_traitlets(self):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you should include docstring to these reset* functions , just to keep it consistent with the rest of your changes where you added docstring

@superstar54
Copy link
Member Author

Hi @AndresOrtegaGuerrero , thanks for the review. I made the changes as you requested.

  • add docstring to the reset methods.
  • remove the message of Stored in AiiDA....

Copy link
Member

@AndresOrtegaGuerrero AndresOrtegaGuerrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! LGTM. One note once you use .reset() for the settings in the panels, if a future plugin doesnt have this method it should be a problem right? I think that just be a different issue for the documentation for plugin developers

@superstar54 superstar54 merged commit fc36106 into main Oct 15, 2023
19 checks passed
@superstar54 superstar54 deleted the feature/reset_GUI branch October 15, 2023 19:26
@superstar54
Copy link
Member Author

One note once you use .reset() for the settings in the panels, if a future plugin doesnt have this method it should be a problem right? I think that just be a different issue for the documentation for plugin developers

Thanks for the comment. In the Panel class, there is an empty reset method to ensure it always exists.

@unkcpz
Copy link
Member

unkcpz commented Oct 16, 2023

Thanks @superstar54. This is finally happened, great!

In the Panel class, there is an empty reset method to ensure it always exists.

However, there is no guarantee that user (property plugin developers) will implement this. I think if the reset is the method that must be overridden otherwise the app is not working properly, you need make the reset of Panel raise NotImplement exception or provide the default behaviour to avoid breaking the app.
If this is the case, please open an issue for future implementation. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create new workflow using workflow selector reset the functional but not update widget
3 participants