search

mantidproject / mantid

Main repository for Mantid code
https://www.mantidproject.org
GNU General Public License v3.0
210 stars 122 forks source link

Replace Euphonic importer test #33156

Closed ajjackson closed 1 year ago

ajjackson commented 2 years ago

The existing test of abins.input.euphonicloader has bee falling over on Windows/Conda, seemingly due to small numerical differences when comparing atomic displacements.

@Pasarus is removing the code

    @unittest.skipUnless(euphonic_available(), 'Optional dependency (euphonic) not available')
    def test_al_phonopy(self):
        self.check(name="Al_LoadPhonopy", loader=EuphonicLoader, extension="yaml")

This should be replaced by a new test to recover the appropriate level of test coverage. The problem is that while previously-implemented formats load atom displacement data from a file (and so are expected to agree exactly), the Euphonic force-constants import generates these by linear algebra methods that can be sensitive to the computing environment. (In particular, degenerate frequencies can lead to equally-valid displacement sets with a different order!)

The existing test uses a pre-existing Abins framework that is designed for exact comparisons of all input data. For this case, a slightly different approach should be used that is more tolerant of numerical differences while still ensuring that the data is valid.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had activity in 6 months. It will be closed in 7 days if no further activity occurs. Allowing issues to close as stale helps us filter out issues which can wait for future development time. All issues closed by stale bot act like normal issues; they can be searched for, commented on or reopened at any point. If you'd like a closed stale issue to be considered, feel free to either re-open the issue directly or contact a developer. To extend the lifetime of an issue please comment below, it helps us see that this is still affecting you and you want it fixed in the near-future. Extending the lifetime of an issue may cause the development team to prioritise it over other issues, which may be closed as stale instead.

ajjackson commented 2 years ago

Euphonic importer testing does warrant another look. Probably going to open a PR soon for more Euphonic-based file import so might make sense to look at then. (If we support direct data import as well as interpolation, then we can have some less numerically-sensitive tests!)

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had activity in 6 months. It will be closed in 7 days if no further activity occurs. Allowing issues to close as stale helps us filter out issues which can wait for future development time. All issues closed by stale bot act like normal issues; they can be searched for, commented on or reopened at any point. If you'd like a closed stale issue to be considered, feel free to either re-open the issue directly or contact a developer. To extend the lifetime of an issue please comment below, it helps us see that this is still affecting you and you want it fixed in the near-future. Extending the lifetime of an issue may cause the development team to prioritise it over other issues, which may be closed as stale instead.

stale[bot] commented 1 year ago

This issue has been closed automatically. If this still affects you please re-open this issue with a comment or contact us so we can look into resolving it.