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

Standardize setting-like widgets with tests #451

Merged
merged 4 commits into from
Sep 4, 2023
Merged

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Aug 30, 2023

I had a discussion with @superstar54 yesterday on how to standardize all such "settings" widgets to make them more uniformed and easy to test.
This PR is an attempt. Let's try to make these two setting widgets as perfect as possible and we can then apply it to other settings.

For the override, I plan to have another PR to remove it for sub-widgets and only keep one for the pw parameters override.

@unkcpz unkcpz marked this pull request as draft August 30, 2023 13:46
@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.86% 🎉

Comparison is base (4bf06f0) 55.82% compared to head (6fe5ff3) 56.68%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #451      +/-   ##
==========================================
+ Coverage   55.82%   56.68%   +0.86%     
==========================================
  Files          27       28       +1     
  Lines        2370     2415      +45     
==========================================
+ Hits         1323     1369      +46     
+ Misses       1047     1046       -1     
Flag Coverage Δ
python-3.10 56.68% <100.00%> (+0.86%) ⬆️
python-3.8 56.73% <100.00%> (+0.86%) ⬆️
python-3.9 56.73% <100.00%> (+0.86%) ⬆️

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

Files Changed Coverage Δ
src/aiidalab_qe/app/configuration/__init__.py 35.80% <ø> (-0.35%) ⬇️
src/aiidalab_qe/app/submission/__init__.py 70.60% <ø> (ø)
src/aiidalab_qe/app/configuration/advanced.py 95.59% <100.00%> (+0.32%) ⬆️
tests/configuration/test_advanced.py 100.00% <100.00%> (ø)
tests/conftest.py 97.87% <100.00%> (+0.04%) ⬆️
tests/test_submit_qe_workchain.py 93.33% <100.00%> (-0.18%) ⬇️

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

@unkcpz unkcpz force-pushed the standard-settings-widget branch 2 times, most recently from 968e09e to 5330735 Compare August 30, 2023 14:06
@unkcpz unkcpz marked this pull request as ready for review August 30, 2023 14:55
@AndresOrtegaGuerrero
Copy link
Member

I had a discussion with @superstar54 yesterday on how to standardize all such "settings" widgets to make them more uniformed and easy to test. This PR is an attempt. Let's try to make these two setting widgets as perfect as possible and we can then apply it to other settings.

For the override, I plan to have another PR to remove it for sub-widgets and only keep one for the pw parameters override.

@unkcpz
Copy link
Member Author

unkcpz commented Aug 30, 2023

I continue on changing TotalCharge widget as well, by this commit 6fe5ff3
I think this one is very worth discussing. @superstar54 was proposed that KpointSettings is not needed but can directly use FloatText widget in AdvancedSettings. During refactoring it I find since it makes more sense that kpoint_settings depend on protocol directly thus with logic it is better to make it a widget to deal with the interaction between widgets.

For the TotalCharge widget, I'll let @AndresOrtegaGuerrero and @superstar54 vote on whether it worth being constructed as a widget. I personally think it should, otherwise the override logic is not that easy to implement properly, as well as reset.

@AndresOrtegaGuerrero
Copy link
Member

AndresOrtegaGuerrero commented Aug 31, 2023

But are we keeping kpoints_distance , right? For the systems we work in empa it is extremely useful to have control over these parameters

@superstar54
Copy link
Member

But are we keeping kpoints_distance , right? For the systems we work in empa it is extremely useful to have control over these parameters

Yes, kpoints_distance is kept and moved to the Advanced setting.

Copy link
Member

@superstar54 superstar54 left a comment

Choose a reason for hiding this comment

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

@unkcpz thanks for the PR. LGTM!

Since this is an intermediate PR, please go ahead and merge. Our final version would be,

  • only one protocol in each panel (e.g advanced setting), this protocol will be linked to the protocol of the basic setting, and trigger the update of the panel.
  • one override for each section in the advanced setting, instead of every parameter. @AndresOrtegaGuerrero feel free to discuss this if you have other options.
  • in the advanced setting, if the parameter is only one value (e.g. kpoint_distance), we just use the basic widget from ipywidget. If the parameters belong to one catalog, (e.g. smearing, and degauss are related to occupation), we create a widget to bundle them together. and expose the output value as a dict.

@unkcpz
Copy link
Member Author

unkcpz commented Aug 31, 2023

Thanks for the green light! I'll have a follow-up PR to bundle things to advanced settings and at the end keep only one override for all such settings.
Now we sort of have an agreement on when to construct a widget from small ones. The rule of thumb is if the small widget has its logic controlled by a parent widget (in our case, the kpoint setting widget is included by Advanced setting and having reset and override logic handled by the advanced setting widget) then there is litte need to create a dedicate widget for it. Only where there is more complex logic that is not able to be managed by the parent widget, create a widget to deal with interactions (in our case, the smearing needs an interface to tell its caller if it is activated).

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.

Looks good to me!

@unkcpz unkcpz merged commit 6716091 into main Sep 4, 2023
12 of 13 checks passed
@unkcpz unkcpz deleted the standard-settings-widget branch September 4, 2023 08:44
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.

None yet

3 participants