Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello!
First of all, let me thank you for writing this fantastic package. It's been quite useful for me and seems very easily extensible and customizable.
Background:
For some crystal symmetries, some common softwares have conventions that construct a q-path that is discontinuous in the Brillouin zone.
E.g. BCC (Spacegroup #229) q-paths have X|R (SeekPath) or H|P (pymatgen)
Problem:
The current plotting joins phonon bands across this discontinuity, although it is mostly only visible if the (plot) linewdith of the band is thicker than the linewidth of the vlines at these q-points.
Solution:
These discontinuities can be found by checking for two adjacent x values in the q-path that have the same value. You can then fix this issue by splitting the x and frequency arrays across each discontinuity and making separate plotting calls for each "split". In the case where there are no q-points with discontinuities, this implementation results in the same plotting behavior as before.
Example:
BCC Iron (Fe) with spin polarization turned off displays imaginary modes. In Fe_229_nospin_original.pdf, note the band connection along H|P. In Fe_229_nospin_updated.pdf, the suggested fix is applied and the previous vertical line joining the bands is not present.
Fe_229_nospin_original.pdf
Fe_229_nospin_updated.pdf
Further comments:
I'd be very happy to address any feedback!
add_dispersion
andadd_alt_dispersion
, but this fix also addresses this issue automatically foradd_multi
. There might be cases where this still occurs for some of the other phonon plotting functions if linestyle is used withscatter
.ax.scatter()
foradd_alt_dispersion
. If I'm completely honest, I'm not sure I understand the use cases forax.scatter
with linestyle vsax.plot
with markers.label_ni
is probably not optimal, although it let me leave thetile_properties()
calls alone.