Skip to content

Add unit tests for fisher function in utils#55

Merged
jfkcooper merged 39 commits intomainfrom
53-add_fisher_test
Aug 1, 2023
Merged

Add unit tests for fisher function in utils#55
jfkcooper merged 39 commits intomainfrom
53-add_fisher_test

Conversation

@sstendahl
Copy link
Collaborator

Fixes issue #53

In this current PR the following unit tests are performed for the fisher function:

  • test_fisher_information_values tests for a known case whether the calculated values are still correct
  • test_fisher_shape Tests whether the shape of the fisher matrix remains correct when using different model parameters
  • test_fisher_diagonal_positive tests for varying sets of q-values whether the diagonal elements are greater than zero. As far as I understood, these diagonal elements represent the FI for the individual parameter on this row, and should never be zero.

Also, the triple assignments in fisher function have been removed, and the associated docstring now indicates the list type for qs and counts (a list of list),

I refactored the general structure of the unit test quite a bit since this morning, moving the get_fisher_information outside of the testing Class, as to allow to test for different cases. Given it's a cheap calculation, it is probably still worth it to just calculate g in each test.

There's probably a few more tests that could be added as well, such as the importance scaling and the gradient calculations. But I figured those may be added in a separate PR:

@sstendahl sstendahl requested a review from jfkcooper July 20, 2023 09:07
Copy link
Owner

@jfkcooper jfkcooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite a few comments here, please message if any are not clear, or if you want further info on how to implement them.

Add docstrings to the break-out functions.
Also add some type hints, give the non-user facing functions an underscore prefix, and revert the instance check to an instance check instead of an attribute check. (Neither is ugly, instance check is probably more robust)
Add some docstrings, and did some minor formatting tweaks to make it slightly more in-line with PEP8 regarding line-length. (I am personally targeting 79 characters (PEP default, but they explictly state that one can deviate if agreed upon), but we probably want to formaly decide this with a linter (e.g. Flake8)
@sstendahl sstendahl requested a review from jfkcooper July 21, 2023 13:04
@sstendahl
Copy link
Collaborator Author

With regard to the latest PR, there's some bigger changes:

  • Two seperate methods can be called to either generate a proper calculation of the Fisher matrix, or two create one with mocked parameters and reflectivity (bypassing any models, simulations and reflectivity calculations).

  • The fisher matrix is tested against known values using the refl1d and refnx model

  • A test is added to check that an empty array of qs gives the proper empty matrix

  • A test is added to check that the Fisher matrix values remain (mostly) the same when changing step size

  • A mock of Xi (the list of parameters) has been created, as opposed to mocking the gradient calculation. The thinking being that the gradient calculation itself should be tested either way, and also that it's nice to be able to give actual dummy values for each parameter in the tests.

  • Some refactoring has been done in fisher function. This was mostly needed to facilitate the patch in get_fisher_mock

Copy link
Owner

@jfkcooper jfkcooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple more to come

@sstendahl
Copy link
Collaborator Author

The refl1d model fixture would need to be refactored a bit once the simulate class is refactored (as far as I can tell, that's the only part in test_fisher.py that actually depends on simulate), but I thought it would make most sense to do that once that refactor is actually in.

I could move the logic that it borrows from simulate to a helper function in test_fisher.py otherwise, but that would basically mean copying that current function to here, which would not be pretty and add several extra dependencies from the simulate class.

@sstendahl sstendahl requested a review from jfkcooper July 28, 2023 15:50
Copy link
Owner

@jfkcooper jfkcooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more comments, getting quite close now.

@sstendahl sstendahl requested a review from jfkcooper August 1, 2023 13:42
Copy link
Owner

@jfkcooper jfkcooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very very minor! sorry

"""
g_reference = get_fisher_information(refl1d_model, step=0.005)
g_compare = get_fisher_information(refl1d_model, step=step)
np.testing.assert_allclose(g_reference, g_compare, rtol=1e-02, atol=0)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one more atol to remove

@sstendahl sstendahl requested a review from jfkcooper August 1, 2023 15:14
Copy link
Owner

@jfkcooper jfkcooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

@jfkcooper jfkcooper merged commit 338a930 into main Aug 1, 2023
@jfkcooper jfkcooper deleted the 53-add_fisher_test branch August 1, 2023 15:18
@sstendahl sstendahl mentioned this pull request Sep 4, 2023
sstendahl pushed a commit to sstendahl/HOGBEN that referenced this pull request Sep 6, 2023
Add unit tests for `fisher` function in `utils`
sstendahl pushed a commit to sstendahl/HOGBEN that referenced this pull request Sep 6, 2023
Add unit tests for `fisher` function in `utils`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants