Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reworking the Slicer Unit Tests #44

Open
ehewins opened this issue Aug 9, 2023 · 1 comment · May be fixed by #47
Open

Reworking the Slicer Unit Tests #44

ehewins opened this issue Aug 9, 2023 · 1 comment · May be fixed by #47

Comments

@ehewins
Copy link
Contributor

ehewins commented Aug 9, 2023

Is your feature request related to a problem? Please describe.
The classes of sasdata/data_util/manipulations.py make up the backend to sasview's slicers. The respective unit tests for these classes are found in test/sasdataloader/utest_averaging.py and these tests leave much to be desired.

There are two classes with tests in utest_averaging.py: Averaging and DataInfoTests.
The tests in Averaging seem largely pointless. For starters only two of the eight slicer averaging methods are tested here. These tests are performed using a 100x100 array of 1s as the test dataset, and the test passes result post-averaging is 1. This does not seem like a good test.
The tests in DataInfoTests are better, and it actually tests the full spectrum of slicers. What is not ideal however is that their data is loaded in from an external file, and so are the expected results (in most cases). I have been told this is not good practice, and it would be better to mathematically construct this data within the test itself. These tests should have an expected answer that can be calculated analytically, probably also from within the test.

There are some other issues too:

  • test_sectorphi_full and test_sectorq_full do not actually test an azimuthal range of 2pi like their docstrings claim.
  • There are no tests for the other aspects of manipulations.py such as the supporting (class agnostic) functions
  • There are no tests to check how the averagers respond to bad input parameters.
  • There are no tests for the mask related classes, Ringcut, Boxcut and Sectorcut.

Describe the solution you'd like
The new unit tests should achieve the following:

  1. Test all of the averagers under several sets of conditions, particularly where potential bugs may arise.
  2. Use test data constructed within the test file. Should be of a mathematically simple form, with expected results that are easily calculated and analytical in form.
  3. Test all general-purpose functions present in manipulations.py that are used by the averagers.
  4. Test the masks? This is lower priority, as I am much less familiar with how these are supposed to work, and how/whether they're implemented in SasView.

Additional context
My end goal is a rework of manipulations.py, and in order to do a good job of this I need to know that each of the averagers is working as intended. The current unit tests in DataInfoTests could be sufficient for this, but making them more robust would be better in the long term.
By writing new unit tests, I'll also become more familiar with the key operations in each of the averaging processes, and therefore better able to simplify and generalise these processes.

@ehewins
Copy link
Contributor Author

ehewins commented Aug 24, 2023

I've now pushed a branch to the repo which contains some new analytical unit tests: 44-new-unit-tests-for-slicer-averagers. The work isn't quite complete yet, as I haven't written any checks for correctly processed error data yet. I'd also planned to test various parts of the code which check the supplied data is valid.
There should be further updates to this test/sasdataloader/utest_averaging_analytical.py file as my rewrite of manipulations.py progresses.

@ehewins ehewins linked a pull request Sep 17, 2023 that will close this issue
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant