-
Notifications
You must be signed in to change notification settings - Fork 9
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
Improve test_mapping_script #84
Comments
The link to the mapping script doesn't work, but I assume it's the emodule-script written by Ilias, on which I am also basing my mapping script for the mixture data. In my script I already replaced the print by loguru. |
@alFrie yes, but I didnt commit my changes as Line 64 is not resolved yet. I need to change the query and then commit all the changes as all of them mentioned in one issue. |
The test mapping script is now getting a rework (#134 ), so should this issue be closed or maybe used for the discussion on the new test script, @eriktamsen ? |
I dont fully follow what is being developed where.
|
In PR #53 @joergfunger mentioned a couple of points to improve the test of the mapping script. I will try to summarize the points and include Jörgs original comments:
Line 10:
mapping is done for different experiments, in that case Youngs modulus test (which is called emodul_mapping). Thus, I would suggest to have a file in test/mapping that is called test_emodul_mapping.py). I would also suggest we stick to a single testing framework (as Erik has already pointed out) and use pytest instead of unittest.
Line 13:
It would be advantageous to not use print but rather the loguru logger such that we can define (when running the tests) the level of output we are interested in.
Line 53:
are these newly created files added to the gitignore
Line 64:
This only checks of there is a single informationbearingentity1 with the value. How do we know that this is the diameter? I would think it might be relevant to start from the id of the test and then extract its diameter traversing the complete graph. This might look redundant (and does not allow to have a query_and_test method) but IMO advantageous (since this is how we are supposed to use the knowledge graph afterwards). This is true for all the subsequent tests.
The text was updated successfully, but these errors were encountered: