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

More testing (plus bug fix) #17

Merged
merged 8 commits into from
Dec 4, 2024
Merged

More testing (plus bug fix) #17

merged 8 commits into from
Dec 4, 2024

Conversation

afmagee42
Copy link
Contributor

@afmagee42 afmagee42 commented Dec 4, 2024

This PR's scope has increased slightly having found the below bug in matrix transposition. It now includes:

  • Making pytest run on PRs
  • A bug fix to matrix transposition in ngm.get_R()
  • A 2x2 test against Keeling and Rohani (passes with bug fix)
  • An additional two correctness tests in the spirit of NGM model demo #8, both of which lean on the newly-added ngm.run_ngm() which repeatedly applies the NGM (per slide 24 here)
    • test_one_step() tests that applying one step of the NGM from the nominal "stationary" distribution:
      1. Does not change the distribution
      2. Grows by a factor of the eigen-value-derived $R$
    • test_eigenvectors() plays out the NGM from one case for many (200) steps and compares to the eigenvector
  • Updates to two existing tests
    • test_vax_beta is a correctness test which had the matrix transposed the same way as get_R
    • test_simulate is a consistency-of-output test, so I re-ran the code and updated the outputs

@swo
Copy link
Contributor

swo commented Dec 4, 2024

I think this might be the problem: https://github.com/cdcent/cfa-ngm-widget/blob/main/ngm/__init__.py#L67

Should be?

return np.transpose(beta) * (s_i / n.sum()) * s_vax

@afmagee42
Copy link
Contributor Author

That does in fact look suspicious. Checking numpy's broadcasting, that looks like it goes into columns, so

>>> np.ones((2,2,)) * np.array([0.25, 0.75])
array([[0.25, 0.75],
       [0.25, 0.75]])

But our docs say it should go into rows https://github.com/cdcent/cfa-ngm-widget/blob/f16d72085fcac4fe4f9af37733e28b2f4fd8750c/docs/approach.md?plain=1#L27, https://github.com/cdcent/cfa-ngm-widget/blob/f16d72085fcac4fe4f9af37733e28b2f4fd8750c/docs/approach.md?plain=1#L43

@afmagee42
Copy link
Contributor Author

But I disagree on the solution, you need to transpose it back again at the end.

return (beta.T * ((s_i / n.sum()) * s_vax)).T

@afmagee42
Copy link
Contributor Author

As of implementing my version in 8e6d602, we pass the KR test and fail several others (test_vax_beta, test_simulate)

@afmagee42
Copy link
Contributor Author

We bootstrapped ourselves to the transposed matrix, though, so that's not entirely surprising?

@afmagee42
Copy link
Contributor Author

Okay @swo, @paigemiller, @dinacmistry, this is ready for a (second) look

@afmagee42 afmagee42 changed the title Add 2x2 test for Keeling and Rohani More testing (plus bug fix) Dec 4, 2024
@afmagee42 afmagee42 merged commit 86e53ac into main Dec 4, 2024
2 checks passed
@afmagee42 afmagee42 deleted the afm-2x2-test branch December 4, 2024 21:07
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 this pull request may close these issues.

3 participants