Repository: pace-neutrons/Euphonic (language: python)

Issue:
Detect repeated q-points in band-structure data
Fixes #202 

When importing QpointFrequencies/QpointPhononModes on a high-symmetry path it is possible that a repeated non-Gamma q-point will not be identified as a turning point. Conventionally these repeated points are used to indicate points-of-interest in band structure data (i.e. places to insert a vertical line and a label).

It seems harmless to check for these while looking for Gamma and turning points and add them to the "qpt_has_label" list, and will occasionally catch a missing point. Sometimes special points are marked in the middle of a high-symmetry line without direction change,

That being said, the example in #202 _does_ have a direction change so I don't quite understand why it isn't caught; maybe there is still a bug there to catch/fix?

- [x] Add a test case
- [x] Check the new behaviour isn't contradicted in the docs somewhere


Discussion / hints:
## Unit Test Results
       19 files  ±  0         19 suites  ±0   39m 3s [:stopwatch:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v1.20/README.md#the-symbols "duration of all tests") + 5m 43s
  1 015 tests +  1    1 010 [:heavy_check_mark:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v1.20/README.md#the-symbols "passed tests") +  1    5 [:zzz:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v1.20/README.md#the-symbols "skipped / disabled tests") ±0  0 [:x:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v1.20/README.md#the-symbols "failed tests") ±0 
10 080 runs  +10  10 009 [:heavy_check_mark:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v1.20/README.md#the-symbols "passed tests") +10  71 [:zzz:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v1.20/README.md#the-symbols "skipped / disabled tests") ±0  0 [:x:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v1.20/README.md#the-symbols "failed tests") ±0 

Results for commit 0c71ae06. ± Comparison against base commit 9113e51a.

[test-results]:data:application/gzip;base64,H4sIADVUxGIC/02MSQ7DIBAEv2JxzmEmeIF8JoIxllBsE7GcrPw9yITgY1W36mCLXU1gjw7lrWMh2dhoTl5F6/bMd97zbPIYzxlwqPgMiag4aO5l39m106LsmkV7GO+d/xmf9lIFUbFVAeTfleqEVVyqJ1+j5LbNxgwMaEJlYNR8IjOS1LwfxKw4ChRi6LUCJKFHxT5fQCDM5g8BAAA=

:recycle: This comment has been updated with latest results.
> That being said, the example in #202 _does_ have a direction change so I don't quite understand why it isn't caught; maybe there is still a bug there to catch/fix?

Just looked into this, in `util.direction_changed`, the vector between q-points `delta` is calculated, which for repeated q-points is zero. To find the relative direction of these vectors the dot product `dot` of each `delta` with the next is calculated, which for the repeated q-points will also be zero! So the test to see if it is more than `tolerance` returns `False`. I can't immediately think of a good way around this, maybe it would be enough for now to state in the `direction_changed` docstring that it wont correctly determine direction changes for repeated points?
Ah, I hadn't considered that the angle comparison with a zero-length segment will be screwy! The fix here, to explicitly include any repeated points, should catch it either way?
I don't see anywhere in the docs we make promises about how these labels will be detected, except in the `get_qpoint_labels` docstring which I have just updated.
Interesting! I guess it's possible to manually start CI after force-push if needed?
Yeah, I forgot you could manually re-run with the run_ci label, but I think synchronize might actually be better (you might have to remove/add the label on each commit I think).

Fix patch:
diff --git a/.github/workflows/build_upload_pypi_wheels.yml b/.github/workflows/build_upload_pypi_wheels.yml
index ac1141354..25337dddc 100644
--- a/.github/workflows/build_upload_pypi_wheels.yml
+++ b/.github/workflows/build_upload_pypi_wheels.yml
@@ -8,11 +8,11 @@ jobs:
   build:
     strategy:
       matrix:
-        os: [windows-2019, macos-latest, ubuntu-latest]
+        os: [windows-latest, macos-latest, ubuntu-latest]
         architecture: [x64]
-        python-version: [3.6, 3.7, 3.8]
+        python-version: [3.6, 3.7, 3.8, 3.9, 3.10]
         include:
-          - os: windows-2019
+          - os: windows-latest
             wheelname: win
           - os: macos-latest
             wheelname: macos
@@ -28,6 +28,12 @@ jobs:
           - python-version: 3.8
             manylinux-version-tag: cp38-cp38
             numpy-version: 1.17.3
+          - python-version: 3.9
+            manylinux-version-tag: cp39-cp39
+            numpy-version: 1.19.3
+          - python-version: 3.10
+            manylinux-version-tag: cp310-cp310
+            numpy-version: 1.21.3
       fail-fast: false
     runs-on: ${{ matrix.os }}
     steps:
@@ -63,7 +69,7 @@ jobs:
           pip-wheel-args: '--no-deps -w wheelhouse'
 
       - name: Create source distribution
-        if: matrix.os == 'windows-2019' && matrix.python-version == '3.8'
+        if: matrix.os == 'windows-latest' && matrix.python-version == '3.10'
         run: |
           python setup.py sdist
         shell: bash
@@ -74,7 +80,7 @@ jobs:
             name: wheels
             path: wheelhouse/*-${{ matrix.wheelname }}*.whl
       - name: Upload source dist to PyPI
-        if: matrix.os == 'windows-2019' && matrix.python-version == '3.8'
+        if: matrix.os == 'windows-latest' && matrix.python-version == '3.10'
         env:
           TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }}
           TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }}
diff --git a/release_tox.ini b/release_tox.ini
index bcbccd240..a90405d89 100644
--- a/release_tox.ini
+++ b/release_tox.ini
@@ -2,7 +2,7 @@
 # Use conda to set up the python environments to run in
 requires = tox-conda
 # The python environments to run the tests in
-envlist = pypi-py36,{pypi,conda}-{py37,py38},conda-{py39,py310},pypisource-{py36,py310}
+envlist = pypi-py36,{pypi,conda}-{py37,py38,py39,py310},pypisource-{py36,py310}
 # Skip the execution of setup.py as we do it with the correct version in commands_pre below
 skipsdist = True
 
@@ -24,7 +24,7 @@ commands_pre =
     --no-binary 'euphonic'
 
 
-[testenv:pypi-{py36,py37,py38}]
+[testenv:pypi-{py36,py37,py38,py39,py310}]
 install_command = python -m pip install {opts} {packages}
 deps =
     numpy
diff --git a/tox.ini b/tox.ini
index bb328face..ee302af44 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,7 +1,7 @@
 [tox]
 requires = tox-conda
 # The python environments to run the tests in
-envlist = py37,py38,py36-{base,matplotlib,phonopy_reader,all},py36-minrequirements-linux
+envlist = py37,py38,py39,py310,py36-{base,matplotlib,phonopy_reader,all},py36-minrequirements-linux
 # Skip the execution of setup.py as we do it with the correct arg in commands_pre below
 skipsdist = True
 
@@ -9,7 +9,7 @@ skipsdist = True
 changedir = tests_and_analysis/test
 test_command = python run_tests.py --report
 
-[testenv:{py37,py38}]
+[testenv:{py37,py38,py39,py310}]
 install_command =
     python -m pip install \
         --force-reinstall \


Test patch:
diff --git a/.github/workflows/run_tests.yml b/.github/workflows/run_tests.yml
index 981997096..4b354aef3 100644
--- a/.github/workflows/run_tests.yml
+++ b/.github/workflows/run_tests.yml
@@ -6,7 +6,7 @@ on:
       - '**_run_ci**'
       - '**release**'
   pull_request:
-    types: [opened, reopened, labeled]
+    types: [opened, reopened, labeled, synchronize]
   workflow_dispatch:
 
 jobs:
@@ -17,7 +17,7 @@ jobs:
        (github.event_name == 'pull_request' && !contains(github.event.pull_request.labels.*.name, 'no_ci'))
     strategy:
       matrix:
-        os: [ubuntu-latest, windows-2019, macos-latest]
+        os: [ubuntu-latest, windows-latest, macos-latest]
       fail-fast: false
     runs-on: ${{ matrix.os }}
     steps:
@@ -33,7 +33,14 @@ jobs:
         run: |
           python -m pip install --upgrade pip
           python -m pip install -r tests_and_analysis/ci_requirements.txt
-      - name: Run tests
+      - name: Run tests, skip Python 3.8, 3.9 unless workflow dispatch
+        if: github.event_name != 'workflow_dispatch'
+        env:
+          TOX_SKIP_ENV: '.*?(py38|py39).*?'
+        shell: bash -l {0}
+        run: python -m tox
+      - name: Run tests, workflow dispatch so test all Python versions
+        if: github.event_name == 'workflow_dispatch'
         shell: bash -l {0}
         run: python -m tox
       - name: Upload test results
diff --git a/.github/workflows/test_release.yml b/.github/workflows/test_release.yml
index ff33b0e6b..807046b60 100644
--- a/.github/workflows/test_release.yml
+++ b/.github/workflows/test_release.yml
@@ -10,7 +10,7 @@ jobs:
   test:
     strategy:
       matrix:
-        os: [ubuntu-latest, windows-2019, macos-latest]
+        os: [ubuntu-latest, windows-latest, macos-latest]
       fail-fast: false
     runs-on: ${{ matrix.os }}
     steps: