Fix tot magnetization #512

AndresOrtegaGuerrero · 2023-10-17T14:20:10Z

This PR solves #412

AndresOrtegaGuerrero · 2023-10-17T14:22:53Z

@unkcpz This is a draft, but in order for me to continue there must be a PR (in aiida-quantumespresso) to deal with the logic that if the system has tot_magnetization , it should remove starting_magnetization for PwBaseWorkChain. Likewise that the logic in PdosWorkChain (Since is a spin polarized calculation with no Fermi) and BandsWorkchain is not affected

unkcpz

Thanks a lot for trying this @AndresOrtegaGuerrero, and sorry for the late review. The logic looks all good, but I think it requires some iron-up on the clarity of implementation. Can you fix the conflict and we arrange a pair coding session to check things together?

src/aiidalab_qe/app/configuration/advanced.py

unkcpz · 2023-11-07T15:12:47Z

src/aiidalab_qe/app/configuration/advanced.py

        parameters["kpoints_distance"] = self.value.get("kpoints_distance")
+


Can you write a comment here for all conditions?
For instance:

# insulator and colliner: xxxx # metal and collinear: xxx

Can the if..else clause below can be simplified with inverse and break return?

unkcpz · 2023-11-07T15:17:35Z

src/aiidalab_qe/app/configuration/advanced.py

+                self.pseudo_setter.structure = None
+                self.pseudo_setter._reset()
+            else:
+                self.pseudo_setter._reset()


Why can not call pseudo_setter.structure = None if input_structure is not None? If I understand correct, the reset should also do self.structure=None first.

src/aiidalab_qe/app/configuration/advanced.py

src/aiidalab_qe/plugins/bands/workchain.py

for more information, see https://pre-commit.ci

AndresOrtegaGuerrero · 2023-11-29T11:58:09Z

To complete this PR the following requirements should be met

PdosWorkChain should be adapted to run calculations using tot_magnetization PdosWorkChain except when tot_magnetization is used aiidateam/aiida-quantumespresso#988
The new BandsPlotWidget New BandsPdosWidget #581
the logic of the qe app should be adapted for having two fermi energies (up/down): get_pdos_data, projections, bands-widget

for more information, see https://pre-commit.ci

codecov · 2024-02-13T15:38:42Z

Codecov Report

Attention: Patch coverage is 66.66667% with 35 lines in your changes are missing coverage. Please review.

Project coverage is 75.47%. Comparing base (97c9c10) to head (a31aeea).

Files	Patch %	Lines
src/aiidalab_qe/common/bandpdoswidget.py	41.17%	20 Missing ⚠️
src/aiidalab_qe/app/configuration/advanced.py	82.45%	10 Missing ⚠️
src/aiidalab_qe/plugins/bands/workchain.py	33.33%	2 Missing ⚠️
src/aiidalab_qe/plugins/pdos/workchain.py	33.33%	2 Missing ⚠️
src/aiidalab_qe/workflows/__init__.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #512      +/-   ##
==========================================
- Coverage   75.73%   75.47%   -0.26%     
==========================================
  Files          60       60              
  Lines        4319     4404      +85     
==========================================
+ Hits         3271     3324      +53     
- Misses       1048     1080      +32

Flag	Coverage Δ
python-3.10	`75.47% <66.66%> (-0.26%)`	⬇️
python-3.9	`75.50% <66.66%> (-0.26%)`	⬇️

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.

for more information, see https://pre-commit.ci

…iidalab-qe into fix_tot_magnetization

for more information, see https://pre-commit.ci

superstar54

@AndresOrtegaGuerrero , thanks for the work! LGTM!

There are two things we could improve:

If users set Magnetism to Off, the Magnetization section could be hidden.
When switching on Magnetism, the pdos plot could has an optios to plot up and down in two sides.

Could you create issues on this? so that we can do these in the future PRs.

superstar54 · 2024-04-03T16:10:40Z

Before merging the PR, two small changes are needed:

the tot_magnetization should be an integer for a fixed occupation.
There are two tot_magnetization when I load the previous job.

AndresOrtegaGuerrero · 2024-04-03T16:44:18Z

@superstar54 thank you for the review, I thought i fixed the duplication i will have a look. Regarding the tot_magnetization , i kept as a Real number since in the pw.x manual is a real.

superstar54 · 2024-04-03T17:22:34Z

In the fixed occupation, if one sets a non-integer value for the tot_magnetization, pw will raise an error. In the smearing case, it will not.

AndresOrtegaGuerrero · 2024-04-03T19:44:32Z

In the fixed occupation, if one sets a non-integer value for the tot_magnetization, pw will raise an error. In the smearing case, it will not.

Then i guess it should be conditioned then , thank you !

If `tot_magnetization` is specified, the results contain a fermi energy for each spin channel. The general support was added in aiidalab#512 and this commit introduces the required changes to the widget.

AndresOrtegaGuerrero added 3 commits October 17, 2023 07:56

updating Magnetization widget, and including logic for insulator

ec7685f

update override logic

34300c7

workaround logic for removing starting_magnetization

4c28d43

unkcpz requested changes Nov 7, 2023

View reviewed changes

AndresOrtegaGuerrero and others added 4 commits November 29, 2023 08:34

update to main

3505ba4

refactor logic

3620437

[pre-commit.ci] auto fixes from pre-commit.com hooks

0f71a48

for more information, see https://pre-commit.ci

refactor logic

7df14c3

AndresOrtegaGuerrero added 2 commits December 4, 2023 11:44

Merge branch 'main' into fix_tot_magnetization

0fc0f15

Merge branch 'main' into fix_tot_magnetization

b44982d

AndresOrtegaGuerrero mentioned this pull request Feb 13, 2024

Support aiida-quantumespresso plugin to 4.5.0 #601

Merged

AndresOrtegaGuerrero and others added 3 commits February 13, 2024 15:22

fixing logic and tests

3d1e13b

[pre-commit.ci] auto fixes from pre-commit.com hooks

be9d218

for more information, see https://pre-commit.ci

fix undefined variable

9467674

AndresOrtegaGuerrero and others added 4 commits February 21, 2024 10:32

Merge branch 'main' into fix_tot_magnetization

d4a2c6e

updating docstring

7c12691

adapting logic of bands and pdos for bandspdoswidget

dad0f35

[pre-commit.ci] auto fixes from pre-commit.com hooks

50d64ac

for more information, see https://pre-commit.ci

AndresOrtegaGuerrero mentioned this pull request Feb 21, 2024

Add fat bands to the BandPdosWidget. #624

Merged

AndresOrtegaGuerrero added 3 commits February 22, 2024 09:21

summary report logic adjusted

c168777

Merge branch 'fix_tot_magnetization' of https://github.com/aiidalab/a…

18cd4ca

…iidalab-qe into fix_tot_magnetization

update test_report

fcc81e5

AndresOrtegaGuerrero marked this pull request as ready for review February 22, 2024 10:54

AndresOrtegaGuerrero requested a review from unkcpz February 22, 2024 10:54

AndresOrtegaGuerrero added this to the v2024.4.0 milestone Feb 22, 2024

Merge branch 'main' into fix_tot_magnetization

a7f5b33

AndresOrtegaGuerrero mentioned this pull request Feb 29, 2024

Plotly bands plot widget #626

Closed

2 tasks

AndresOrtegaGuerrero requested a review from superstar54 March 5, 2024 09:27

AndresOrtegaGuerrero and others added 3 commits March 22, 2024 11:34

Merge branch 'main' into fix_tot_magnetization

68059cc

Merge branch 'main' into fix_tot_magnetization

83e3e86

[pre-commit.ci] auto fixes from pre-commit.com hooks

6a9cd71

for more information, see https://pre-commit.ci

superstar54 approved these changes Apr 3, 2024

View reviewed changes

AndresOrtegaGuerrero added 4 commits April 5, 2024 09:12

function naming improve

76afdbf

changing tot_magnetization to only Integer numbers

c8e5bdd

starting_magnetization as default

9069437

display kinds by default

a31aeea

AndresOrtegaGuerrero enabled auto-merge (squash) April 5, 2024 12:36

AndresOrtegaGuerrero requested a review from superstar54 April 5, 2024 12:38

superstar54 disabled auto-merge April 5, 2024 12:42

superstar54 merged commit 2c5ca55 into main Apr 5, 2024
19 checks passed

superstar54 deleted the fix_tot_magnetization branch April 5, 2024 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tot magnetization #512

Fix tot magnetization #512

AndresOrtegaGuerrero commented Oct 17, 2023

AndresOrtegaGuerrero commented Oct 17, 2023 •

edited

Loading

unkcpz left a comment •

edited

Loading

unkcpz Nov 7, 2023

unkcpz Nov 7, 2023

AndresOrtegaGuerrero commented Nov 29, 2023 •

edited

Loading

codecov bot commented Feb 13, 2024 •

edited

Loading

superstar54 left a comment •

edited

Loading

superstar54 commented Apr 3, 2024

AndresOrtegaGuerrero commented Apr 3, 2024

superstar54 commented Apr 3, 2024

AndresOrtegaGuerrero commented Apr 3, 2024

		parameters["kpoints_distance"] = self.value.get("kpoints_distance")

Fix tot magnetization #512

Fix tot magnetization #512

Conversation

AndresOrtegaGuerrero commented Oct 17, 2023

AndresOrtegaGuerrero commented Oct 17, 2023 • edited Loading

unkcpz left a comment • edited Loading

Choose a reason for hiding this comment

unkcpz Nov 7, 2023

Choose a reason for hiding this comment

unkcpz Nov 7, 2023

Choose a reason for hiding this comment

AndresOrtegaGuerrero commented Nov 29, 2023 • edited Loading

codecov bot commented Feb 13, 2024 • edited Loading

Codecov Report

superstar54 left a comment • edited Loading

Choose a reason for hiding this comment

superstar54 commented Apr 3, 2024

AndresOrtegaGuerrero commented Apr 3, 2024

superstar54 commented Apr 3, 2024

AndresOrtegaGuerrero commented Apr 3, 2024

AndresOrtegaGuerrero commented Oct 17, 2023 •

edited

Loading

unkcpz left a comment •

edited

Loading

AndresOrtegaGuerrero commented Nov 29, 2023 •

edited

Loading

codecov bot commented Feb 13, 2024 •

edited

Loading

superstar54 left a comment •

edited

Loading