Adding unit testing for ModelSystem and AtomsState #15
Conversation
|
@ladinesa @ndaelman-hu before continuing adding tests, would you mind checking these I prepared? Do they align with the idea you have for doing unit testing? When preparing them, I wanted to test each of the functions defined for the base classes (including |
Great, thanks a lot! Indeed, I am aligning the repo with the other plugins #14, so I will check the coverage. |
JosePizarro3
force-pushed
the
9-adding-unit-testing
branch
2 times, most recently
from
fed99f2 to
9027215
Compare
Pull Request Test Coverage Report for Build 8876276556Details
💛 - Coveralls |
|
@ndaelman-hu would you mind having a look at this? I prepared some testing for |
JosePizarro3
force-pushed
the
9-adding-unit-testing
branch
from
adedb05 to
7292bd6
Compare
|
@ndaelman-hu would you mind taking a look on this? Just a quick check, I have to still rebase to your conftest, and keep writing tests for ModelSystem, it is more the general structure, if you agree or not. |
tests/test_atoms_state.py
Outdated
| @pytest.mark.parametrize( | ||
| 'number, values, results', | ||
| [ | ||
| ('n_quantum_number', [-1, 0, 1, 2], [False, False, True, True]), | ||
| ('l_quantum_number', [-2, 0, 1, 2], [False, False, True, True]), | ||
| # l_quantum_number == 2 when testing 'ml_quantum_number' | ||
| ('ml_quantum_number', [-3, 5, -2, 1], [False, False, True, True]), | ||
| ('ms_quantum_number', [0, 10, -0.5, 0.5], [False, False, True, True]), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
I would extend the coverage here, since you can be exhaustive (at least for the True responses: the False are a pain).
Maybe a series of nested for-loops are more appropriate here than the pytest.mark.parametrize decorator?
Question: what about states that contain multiple electrons?
There was a problem hiding this comment.
I mean now you've only covered n=2.
There was a problem hiding this comment.
Question: what about states that contain multiple electrons?
Here I am only defining orbital states, not the electronic one. I think the electronic could be define a posteriori using some normalization, that's why I open a discussion #22, so that we can safely define electronic state from OrbitalsState
| [ | ||
| ([3.0, 2.0, 1.0], (0.1429146, -0.0357286, 0.0893216)), | ||
| (None, (None, None, None)), | ||
| ([3.0, 2.0, 1.0, 0.5], (None, None, None)), |
There was a problem hiding this comment.
What does this last example test? An incorrect no. passed integrals by whom?
I'd rather see more integrals tested..?
There was a problem hiding this comment.
well, but the functionality does not support it, there is a warning in the function.
JosePizarro3
force-pushed
the
9-adding-unit-testing
branch
7 times, most recently
from
d3a766f to
19a3b86
Compare
Moved logger to tests/__init__.py
Added TestChemicalFormula and fix datamodel
Added testing for Symmetry.normalize Fix model_system.py
…del_system.py classes
Improved testing in test_atoms_state.py
JosePizarro3
force-pushed
the
9-adding-unit-testing
branch
from
1125cd0 to
bae8ccd
Compare
Fix normalize in model_system.py
|
@ladinesa would you mind giving a last final quick look? I understand this pr is big so it will take time, but I am just aiming for a quick double-check (the tests are already thought of). I can then continue adding test to the ModelMethod. |
Adding unit testing for the
model_system.pyandatoms_state.py