Add comprehensive test suite for Feed-Forward Neural Network wavefunction (David’s implementation)#164
Conversation
kzsigmond
left a comment
kzsigmond
left a comment
There was a problem hiding this comment.
Thanks for writing the tests for RBM! Could you please go over the comments?
| @pytest.mark.xfail(reason="pspace=None not supported yet") | ||
| def test_normalize_none(): | ||
| rbm = RestrictedBoltzmannMachine(nelec=2, nspin=4, nbath=2) | ||
| rbm.normalize(pspace=None) |
There was a problem hiding this comment.
Is the development of pspace=None on a roadmap? If the code isn't working for pspace=None, I would prefer to have a value error if pspace is None.
There was a problem hiding this comment.
At the moment, I'm not developing additional features on top of David’s original implementation. To preserve the original design until we have a clearer roadmap for future development, I have intentionally avoided modifying this behavior.
That said, if you feel that explicitly raising an error for pspace=None would improve clarity and safety, I can certainly make that change for now.
There was a problem hiding this comment.
I'd prefer if you would raise an error with a message like: "This feature is not yet implemented". You can keep the code as is in there, I just don't want someone to think that pspace=None is supposed to work, when it doesn't.
| def test_assign_params_flat_and_influences_overlap(): | ||
| rbm = RestrictedBoltzmannMachine(nelec=2, nspin=4, nbath=2, orders=(1,)) | ||
| flat = np.zeros(rbm.nparams); flat[0] = 0.2 | ||
| rbm.assign_params(flat) | ||
| olp0 = rbm.get_overlap(0b0) | ||
| olp1 = rbm.get_overlap(0b1) | ||
| assert olp0 == pytest.approx(1.0) | ||
| assert not np.isclose(olp1, 1.0) |
There was a problem hiding this comment.
Why should the overlap be 1.0 with an SD that has 0 electrons and a wfn that has 2? Shouldn't it be 0?
There was a problem hiding this comment.
The test is correct in its place.
The overlap is one in the case of zero electrons because overlap = Π_j σ( w_j · m ) * output_scale, where σ(x) = 1 + exp(x) and output_scale = 0.5.
If the correct physical behavior is expected, then there is a need to enforce:
get_overlap(sd)=0 when popcount(sd) != nelec- or incorporate antisymmetry.
There was a problem hiding this comment.
Got it! Do you see any potential issues with the "incorrect" physical behavior that could come up when we are calculating the overlap? I think this could potentially happen in two scenarios:
- The user defined p-space contains SDs with different number of electrons. No idea why we would do that, but someone could in theory try it.
- There is some kind of bug in the code elsewhere that changes the number of electrons in the p-space/s-space, which could go unnoticed for RBM.
It might be worth opening up a separate issue on GitHub, where we can address this down the line.
| def test_assign_params_invalid_length(): | ||
| rbm = RestrictedBoltzmannMachine(nelec=2, nspin=4, nbath=2) | ||
| original = rbm.params.copy() | ||
| longer = np.zeros(rbm.nparams + 1) | ||
| rbm.assign_params(longer) # should not crash | ||
| # Parameters should still have expected shape and size | ||
| assert rbm.params.shape == original.shape | ||
| assert np.isfinite(rbm.params).all() |
There was a problem hiding this comment.
Is there a specific reason why we can assign parameters that are too long and then get truncated?
There was a problem hiding this comment.
There is no conceptual reason this wavefunction should accept parameter arrays that are longer than required. The current behavior, silently truncating extra entries, is simply a consequence of how David originally wrote the slicing logic inside assign_params.
If stricter behavior is preferred, an explicit length check is required in place, so that assigning too-long arrays raises a ValueError instead of being silently truncated.
There was a problem hiding this comment.
I am fine with the current behavior if there is some kind of warning statement notifying the user that the provided parameters got truncated.
Thanks for the review. I have updated the tests considering the comments and have already pushed the changes. |
| @pytest.mark.xfail(reason="pspace=None not supported yet") | ||
| def test_normalize_none(): | ||
| rbm = RestrictedBoltzmannMachine(nelec=2, nspin=4, nbath=2) | ||
| rbm.normalize(pspace=None) |
There was a problem hiding this comment.
I'd prefer if you would raise an error with a message like: "This feature is not yet implemented". You can keep the code as is in there, I just don't want someone to think that pspace=None is supposed to work, when it doesn't.
| def test_assign_params_flat_and_influences_overlap(): | ||
| rbm = RestrictedBoltzmannMachine(nelec=2, nspin=4, nbath=2, orders=(1,)) | ||
| flat = np.zeros(rbm.nparams); flat[0] = 0.2 | ||
| rbm.assign_params(flat) | ||
| olp0 = rbm.get_overlap(0b0) | ||
| olp1 = rbm.get_overlap(0b1) | ||
| assert olp0 == pytest.approx(1.0) | ||
| assert not np.isclose(olp1, 1.0) |
There was a problem hiding this comment.
Got it! Do you see any potential issues with the "incorrect" physical behavior that could come up when we are calculating the overlap? I think this could potentially happen in two scenarios:
- The user defined p-space contains SDs with different number of electrons. No idea why we would do that, but someone could in theory try it.
- There is some kind of bug in the code elsewhere that changes the number of electrons in the p-space/s-space, which could go unnoticed for RBM.
It might be worth opening up a separate issue on GitHub, where we can address this down the line.
| def test_assign_params_invalid_length(): | ||
| rbm = RestrictedBoltzmannMachine(nelec=2, nspin=4, nbath=2) | ||
| original = rbm.params.copy() | ||
| longer = np.zeros(rbm.nparams + 1) | ||
| rbm.assign_params(longer) # should not crash | ||
| # Parameters should still have expected shape and size | ||
| assert rbm.params.shape == original.shape | ||
| assert np.isfinite(rbm.params).all() |
There was a problem hiding this comment.
I am fine with the current behavior if there is some kind of warning statement notifying the user that the provided parameters got truncated.
| vec_derivs = rbm.get_overlaps(sds, deriv=deriv_index) | ||
| numeric_grad = finite_diff_grad(rbm, sds[0]) | ||
|
|
||
| assert np.isclose(vec_derivs[0], numeric_grad[deriv_index], |
There was a problem hiding this comment.
| assert np.isclose(vec_derivs[0], numeric_grad[deriv_index], | |
| assert np.isclose(vec_derivs[deriv_index], numeric_grad[deriv_index], |
Not having deriv_index hardcoded for vec_derivs could prevent future issues, if someone decides to change deriv_index to something else.
| ## Derivatives w.r.t multiple params | ||
| #for deriv_index in [0, rbm.nparams - 1]: | ||
| # derivs = rbm.get_overlaps(sds, deriv=deriv_index) | ||
| # assert derivs.shape == (len(sds),) | ||
| # sd = sds[0] | ||
| # eps = 1e-6 | ||
| # base_flat = rbm.params.copy() | ||
| # numeric = finite_diff_grad(rbm, sd)[deriv_index] | ||
| # rbm.assign_params(base_flat) | ||
| # assert np.isclose(derivs[0], numeric, rtol=1e-3, atol=1e-6) |
There was a problem hiding this comment.
Do you want to keep the old code for future reference?
2 participants
This PR introduces a new test file covering the Feed-Forward Neural Network (FFNN) wavefunction implemented by David.