Open
Conversation
Delete one of the hash test files and put tests into a class method under one file. Change print statements to assert statements.
SiPANN is needed in the `py` virtual environment.
SiPANN needed for SiPANN hash tests. numpy version of at least 1.22 needed to avoid nlopt import issues (See here: https://githubhot.com/repo/stevengj/nlopt/issues/447)
Python 3,8 is required to install numpy>=1.22.0, which is needed yo avoid nlopt import errors while attempting to import sipann.
Python 3.8+ is needed to install numpy>=1.22.0
AustP
suggested changes
May 20, 2022
simphony/tests/test_models.py
Outdated
| assert wg1.circuit == wg2.circuit | ||
| assert wg2.circuit == wg3.circuit | ||
| assert brancher.__hash__() == brancher2.__hash__() | ||
| assert brancher.__hash__() != brancher3.__hash__() |
Contributor
There was a problem hiding this comment.
You don't need to have any tests here. The other tests you have are sufficient.
| assert (hr1siepic.__hash__() == hr2siepic.__hash__()) | ||
|
|
||
| hr1siepic = siepic.HalfRing(gap=8e-8, radius=10e-6, width=5.2e-7, thickness=2.1e-7) | ||
| hr2siepic = siepic.HalfRing(gap=1e-7, radius=1e-5, width=5e-7, thickness=2.2e-7) |
Contributor
There was a problem hiding this comment.
For components with multiple parameters, you should probably do multiple tests where each test only has 1 parameter different. That way you can get full coverage.
simphony/tests/test_hash.py
Outdated
|
|
||
| bdc1 = siepic.BidirectionalCoupler(thickness=2.2e-7, width=5e-7) | ||
| bdc2 = siepic.BidirectionalCoupler(thickness=2.2e-7, width=5e-7) | ||
| assert (bdc1.__hash__() == bdc2.__hash__()) |
Contributor
There was a problem hiding this comment.
Remove the extra layer of parenthesis on this assert and all of the rest to match the rest of the testing code.
AustP
approved these changes
Jul 19, 2022
Collaborator
|
Can you remind me what this is supposed to be for? @SkandanC |
Contributor
Author
|
Austen wanted me to add |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR after SiPANN dependency conflict fix merge.