BAMresearch / LebeDigital

The LeBeDigital Concrete Production and Testing Ontology - CPTO Repository
https://bamresearch.github.io/LebeDigital/newest
MIT License
8 stars 9 forks source link

Improve test_mapping_script #84

Open eriktamsen opened 1 year ago

eriktamsen commented 1 year ago

In PR https://github.com/BAMresearch/LebeDigital/pull/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.

alFrie commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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.