fix bug on band_kpoints_distance for periodicity x and xy

[AndresOrtegaGuerrero]

this PR fixes a bug on the bands (summary_viewer.py) logic when running a bands for periodicity x and xy. The bug rises for not having bands_kpoints_distance in the input for periodicity x and xy

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ff5f2cc) 80.72% compared to head (902dd33) 80.77%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #547      +/-   ##
==========================================
+ Coverage   80.72%   80.77%   +0.04%     
==========================================
  Files          49       49              
  Lines        3414     3412       -2     
==========================================
  Hits         2756     2756              
+ Misses        658      656       -2     

Flag	Coverage Δ
python-3.10	?
python-3.8	80.77% <100.00%> (+<0.01%)	⬆️
python-3.9	80.77% <100.00%> (+<0.01%)	⬆️

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

Files	Coverage Δ
src/aiidalab_qe/app/result/summary_viewer.py	75.67% <100.00%> (+0.33%)	⬆️

... and 2 files with indirect coverage changes

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

[unkcpz]

Thanks @AndresOrtegaGuerrero. But I guess there may be a more proper solution for this, let's see how @superstar54's idea.

[unkcpz]

Emmm.... Where are those values come from? I think we need a centrailse place for such value, is qeapp.yaml a good place for it? If it is from the aiida-quantumespresso protocol, then there is PwBaseWorkChain.get_protocol_inputs(protocol) to get it rather than define it again here.

[AndresOrtegaGuerrero]

@unkcpz so these values come from the protocol from bands in the quantum espresso plugin. For 1D and 2D we have to remove bands_kpoints_distance from the inputs.

[unkcpz]

My point is if it is from protocol, you can read it by PwBaseWorkChain.get_protocol_inputs(protocol), is that work?

[AndresOrtegaGuerrero]

I modified as you suggested , and it works :)

[unkcpz]

@superstar54 there are still hardcoded code for bands and pdos here, are these able to be removed by the plugin implementation?

[superstar54]

I think there are two ways to show the plugin's summary info.

• Add an entry point in the Summary panel, which can append a new section for each plugin. The plugin developer only needs to implement a get_report function (like the get_builder function in the workchain entry point). This function takes the ui_parameters and the workchain's output as inputs, and returns a dict to be printed out.
• The plugin developer adds a new tab in the plugin's result panel. and we don't need to change anything on the qeapp side.

What are your opinions? @AndresOrtegaGuerrero @unkcpz

[AndresOrtegaGuerrero]

I think there are two ways to show the plugin's summary info.

• Add an entry point in the Summary panel, which can append a new section for each plugin. The plugin developer only needs to implement a get_report function (like the get_builder function in the workchain entry point). This function takes the ui_parameters and the workchain's output as inputs, and returns a dict to be printed out.
• The plugin developer adds a new tab in the plugin's result panel. and we don't need to change anything on the qeapp side.

What are your opinions? @AndresOrtegaGuerrero @unkcpz

In the first one, the entry can just append in the same tab of 'Workflow Summary' without generating another tab? , Because if that is the case, is nice since there you can get like different sections of the options for each plugin, like one for pdos, for bands, and for future ones.

I am assuming this requires a different PR to address this?, right @superstar54 ?
If this is the case, then at least we should provide a temp solution to enable displaying bands for 1D and 2D systems, currently with a bug.

[superstar54]

the entry can just append in the same tab of 'Workflow Summary' without generating another tab?

Yes

I am assuming this requires a different PR to address this?

Yes.

then at least we should provide a temp solution to enable displaying bands for 1D and 2D systems, currently with a bug.

I agree.

[unkcpz]

If this is the case, then at least we should provide a temp solution to enable displaying bands for 1D and 2D systems, currently with a bug.

I agree, please open an issue and add a comment so we remember to clean up the code after the plugin is there in the future.

[AndresOrtegaGuerrero]

@unkcpz @superstar54 do you maybe know why i cant get all the checks to be completed?

[AndresOrtegaGuerrero]

If this is the case, then at least we should provide a temp solution to enable displaying bands for 1D and 2D systems, currently with a bug.

I agree, please open an issue and add a comment so we remember to clean up the code after the plugin is there in the future.

The issue is #549

[unkcpz]

do you maybe know why i cant get all the checks to be completed?

There is only one test that failed which is arm64 test. I guess it was some problem on the self-runner (Marnik's mac workstation), I'll check it after. There is nothing to worry about if the amd64 checks are passed, hopefully.

[unkcpz]

I think this is not correct, this will make the report distance only rely on the protocol.
The if..else is still needed as your previous change.
If the user sets the bands_kpoints_distance, it needs to be updated in the report. Am I missing something?

[AndresOrtegaGuerrero]

So far the user doesnt have the option to modify bands_kpoints_distance from the bands plugin (qe app)

[unkcpz]

Good! Then it is all fine for me.

[unkcpz]

@AndresOrtegaGuerrero thanks!