aiida-vasp / parsevasp

A general parser for VASP
MIT License
13 stars 13 forks source link

doscar.py reads the pdos incorrectly #118

Closed adair-nicolson closed 2 years ago

adair-nicolson commented 2 years ago

The pdos for each ion is set to the pdos of the first ion in the DOSCAR

https://github.com/aiida-vasp/parsevasp/blob/db19067dac597a477d52691b4f8aecdd3d31e1f3/parsevasp/doscar.py#L220-L223

I think this is the source of the error. During the for loop the pdos of the first ion in the DOSCAR is read in for each ion. I have a fix, but I don't know if it is the most elegant solution.

if line_2 in data:
    start = data.index(line_2) + 1
        for _ in range(num_ions):
            pdos_items += [data[start:start + ndos]]
            start += (ndos+1)
zhubonan commented 2 years ago

Looks good to me. I guess this has gone unnoticed because in the test DOSCAR only contains one atom.... @espenfl

espenfl commented 2 years ago

@adair-nicolson Thanks a lot for detecting and submitting this fix. I agree with @zhubonan.

espenfl commented 2 years ago

@adair-nicolson Can you also update the test that fail? Let me know if you need some help with it.

codecov[bot] commented 2 years ago

Codecov Report

Base: 80.40% // Head: 80.41% // Increases project coverage by +0.01% :tada:

Coverage data is based on head (42c6cae) compared to base (db19067). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #118 +/- ## =========================================== + Coverage 80.40% 80.41% +0.01% =========================================== Files 13 13 Lines 3443 3444 +1 =========================================== + Hits 2768 2769 +1 Misses 675 675 ``` | [Impacted Files](https://codecov.io/gh/aiida-vasp/parsevasp/pull/118?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiida-vasp) | Coverage Δ | | |---|---|---| | [parsevasp/doscar.py](https://codecov.io/gh/aiida-vasp/parsevasp/pull/118/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiida-vasp#diff-cGFyc2V2YXNwL2Rvc2Nhci5weQ==) | `91.18% <100.00%> (+0.09%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiida-vasp). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiida-vasp)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

zhubonan commented 2 years ago

@espenfl I think this is ready to go now!

espenfl commented 2 years ago

@adair-nicolson Thanks a lot. Merged. Sorry for the delay.