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

Improve test_mapping_script #84

Open
eriktamsen opened this issue Nov 23, 2022 · 4 comments
Open

Improve test_mapping_script #84

eriktamsen opened this issue Nov 23, 2022 · 4 comments

Comments

@eriktamsen
Copy link
Member

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:

  • rename mapping files
  • use pytest not unittest package

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:

  • use loguru logger not print in tests

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:

  • open question

are these newly created files added to the gitignore

Line 64:

  • change the query for values

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.

@alFrie
Copy link
Collaborator

alFrie commented Dec 13, 2022

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.
For this script, Soudeh said she already resolved the issue in line 10 and 13, correct @soudehMasoudian ?

@soudehMasoudian
Copy link
Collaborator

@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.

@alFrie
Copy link
Collaborator

alFrie commented Mar 14, 2023

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 ?

@eriktamsen
Copy link
Member Author

I dont fully follow what is being developed where.

  1. Is the work done in branch 84 which is linked to this issue still used? If not, close this issue, delete the branch. Otherwise, merge the branch, and this issue will be closed automatically.
  2. what are branches "soudehMasoudian-patch-1" and "soudehMasoudian-patch-2"? Looks like it is something created automatically?
    I would prefer to open a new issue if new work is done on the mapping, then create a new branch from there which is linked and has the same number and then create a pull request. That way everything stays organized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants