Update/validator #48
Merged
Update/validator #48
Conversation
This file contains 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
|
@ClemensLinnhoff can you review please? |
Yes, I will review this next week. |
|
I cannot extract the small_test sample trace file. |
|
When running osi-validation with a different trace file, I get the following error: I am using pip 20.0.2 and python3.8 in venv. |
|
If I see this correctly, the KPIs folder is not changed by this PR. However, there are mistakes in it. Both angles should be double values and the Requrements are also wrong. See official documentaion: Furthermore, there seems to be a mix-up between RadarSensorView::Reflection and LidarSensorView::Reflection Are the KPIs used for the validation? If yes, they should be corrected, if no, they should be deleted. |
|
Please implement a build check for the GitHub CI pipeline. The travis build check does not seem to work. |
|
The links in the Readme to the documentation do not work. This way, I don't know, which trace file formats are supported. For the .osi trace file: For the .txth trace file: |
|
You write in this PR, to run osi-validation with However, the --data option is not listed in the README. |
Good point! I will migrate the CI from travis to github workflow. |
The KPI folder is not used. I will delete it. |
Can you try?: I will commit the decoded trace to the repo to avoid this issue. |
With |
jdsika
reviewed
Jan 9, 2024
jdsika
reviewed
Jan 9, 2024
jdsika
reviewed
Jan 9, 2024
|
Ah, instructions for the protoc compiler are missing in the readme |
…s. Used Python 3.8 in workflow. Signed-off-by: Carlo van Driesten <[email protected]>
Signed-off-by: Carlo van Driesten <[email protected]>
Signed-off-by: Carlo van Driesten <[email protected]>
Signed-off-by: Carlo van Driesten <[email protected]>
jdsika
reviewed
Jan 9, 2024
|
!!!! setup.py does have a different requirements list then the requirements.txt !!!! |
|
Signed-off-by: ClemensLinnhoff <[email protected]>
|
@vkresch you have removed lzma encoding but it is still present in the OSI repository: https://github.com/OpenSimulationInterface/open-simulation-interface/blob/master/format/OSITrace.py Update: Please create a ticket in OSI to remove lzma there in the scripts as well |
… Fix requirements Signed-off-by: Carlo van Driesten <[email protected]>
Signed-off-by: Carlo van Driesten <[email protected]>
… instructions Signed-off-by: Carlo van Driesten <[email protected]>
|
@vkresch my mission was successful. I killed the tests because of the minimum proto version from the OSI repository :)) |
|
Nice :) this was my concern with the bumping. |
jdsika
reviewed
Jan 11, 2024
…n in requirements.txt and setup.py Signed-off-by: Carlo van Driesten <[email protected]>
|
You downloaded a different (unknown) version of the protoc compiler in the ci workflow. I now updated it to be the same as in the osi repository. |
|
Twitter showing this after my review: |
|
Thanks for the fix! |
…ump year in license, add git+osi requirement Signed-off-by: Carlo van Driesten <[email protected]>
Signed-off-by: Carlo van Driesten <[email protected]>
jdsika
approved these changes
Jan 15, 2024
|
@jdsika looks like I do not have permission to merge the PR. I noticed an issue with the parallel run option and fixed it. Please review it again. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves #49
Resolves #46
Resolves #45
Resolves #44
Resolves #43
Resolves #42
Resolves #31
Change Notes:
rulesfolder fromrequirements-osi-3folderHow has it been tested?
I tested the rules with defining the x position range of all moving objects and it gave me validation errors outside this range for the
small_test.txt. In the test trace there is only one moving object which moves in the positive x direction. To reproduce go intoosi_common.yamland change to this:Validation Result:
Mention a member
@jdsika please check the latest update
in terminal you should be able to install with:
$ pip install .and to run with:
$ osivalidator --data data/small_test.txt --rules rules/Check the checklist
To Fix
Enable reading human readable trace filesEnable setting a rule to define, if a field is required. Giving a warning for every missing field is not really effective.-> Enable setting a rule to define, if a field is required #51Fix that when a rule is deleted, it is not checked anymore.-> A field removed from the rules is still tested #52Fix logging to correctly differentiate between error and warning analog to test results.-> Fix logging to correctly differentiate between error and warning analog to test results #53