Skip to content

Validating Dtypes from String #370

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 25 commits into from
Mar 25, 2020
Merged

Validating Dtypes from String #370

merged 25 commits into from
Mar 25, 2020

Conversation

fschrader1992
Copy link
Collaborator

@fschrader1992 fschrader1992 commented Mar 19, 2020 •
edited
Loading

This PR

  • adds validations fro property values with dtype string. It checks, whether possible other dtypes might have been detected as string instead and issues a warnings.
  • adds corresponding tests.
  • adds new xml, yaml and json files to test respective validations.

Checks for Code Quality:

  • Pylint
  • Rebase from GNODE Master
  • Repeat Local Tests Python3.8
  • Repeat Local Tests Python2.7

@fschrader1992
[validation.py] Add Possible DType Check for String Property Values
d7dd02d 
Checks, whether a Property with dtype string might be a different dtype but could not be detected
when reading a file or property.
@fschrader1992
[test_validation.py] Add Test for Property String Dtype Validation
be4663c
@fschrader1992
[test/resources] Add integration.xml for odML Validation Test
2ec7e54
@fschrader1992
[test_validation.py] Add Test for XML File Validation
53eba68
@fschrader1992
[test/resources] Add integration.json for odML Validation Test
d062f52
@fschrader1992
[test_validation.py] Add Test for JSON File Validation
3e5aaa7
@fschrader1992
[test/resources] Add integration.yaml for odML Validation Test
43b4941
@fschrader1992
[test_validation.py] Add Test for YAML File Validation
30892b4
@fschrader1992
[test_validation.py] Specify Section Type Test for File Loading Tests
f24eed3
@fschrader1992
[test_validation.py] Add Test for Property String Dtype Validation
b5d8d09
@coveralls
Copy link

coveralls commented Mar 19, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 75.627% when pulling baf75c8 on fschrader1992:validation into 48d83b7 on G-Node:master.

mpsonntag
mpsonntag requested changes Mar 23, 2020
View reviewed changes
Copy link
Contributor

@mpsonntag mpsonntag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice PR, thanks!

I would like to ask for a change in the validation behaviour though. I think it would be better, if the first value is used to identify a dtype different from string but only suggest a dtype change, if all values fit the suggested dtype. If the values are a mix of different dtypes, a change in dtype should not be suggested.

@fschrader1992
[validation.py] Throw String Dtype Warning Only If All Values Match D…
e22f725 
…type
@fschrader1992
[test_validation.py] Move dir_path to setUp
f912971
@fschrader1992
[test_validation.py] Adapt Test for String Dtype Validation Behavior
dde6730 
Show warning only if all values fit dtype.
@fschrader1992
[test_validation.py] Add Property with Dtype String for Property Stri…
0d41ae8 
…ng Values Test
@fschrader1992
[test/resources] Replace File for XML Validation Tests
4160a8b 
Remove integration.xml
Add validation_dtypes.xml
Add validation_section.xml

Split test functionality.
@fschrader1992
[test_validation.py] Add Test for XML File Loading Sections
7ee2ef6 
- Replace part of current XML File Loading Test
- Incorporate new file in resources/validation_section.xml
@fschrader1992
[test_validation.py] Add Test for XML File Loading Property Dtypes
4b1939a 
- Replace current XML File Loading Test
- Incorporate file at resources/validation_dtypes.xml
@fschrader1992
[test/resources] Replace File for JSON Validation Tests
3c78947 
Remove integration.json
Add validation_dtypes.json
Add validation_section.json

Split test functionality.
@fschrader1992
[test_validation.py] Add Test for JSON File Loading Sections
719e6fb 
- Replace part of current JSON File Loading Test
- Incorporate file at resources/validation_section.json
@fschrader1992
[test_validation.py] Add Test for JSON File Loading Property Dtypes
2488ceb 
- Replace current JSON File Loading Test
- Incorporate file at resources/validation_dtypes.json
@fschrader1992
[test/resources] Replace File for YAML Validation Tests
976b10b 
Remove integration.yaml
Add validation_dtypes.yaml
Add validation_section.yaml

Split test functionality.
@fschrader1992
[test_validation.py] Add Test for YAML File Loading Sections
f85ffe6 
- Replace part of current YAML File Loading Test
- Incorporate file at resources/validation_section.yaml
@fschrader1992
[test_validation.py] Add Test for YAML File Loading Property Dtypes
52eb412 
- Replace current YAML File Loading Test
- Incorporate file at resources/validation_dtypes.yaml
@fschrader1992
[validation_dtypes.json] Remove Wrongly Set Dtype String
fd78be4
@fschrader1992
[validation_dtypes] Remove Section For Text Without Dtype
baf75c8 
Unnecessary for validation tests.
@fschrader1992
Copy link
Collaborator Author

Checks for Code Quality:

  • Pylint
  • Rebase from GNODE Master
  • Repeat Local Tests Python3.8
  • Repeat Local Tests Python2.7

@mpsonntag mpsonntag merged commit 7e9299e into G-Node:master Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpsonntag mpsonntag mpsonntag requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fschrader1992 @coveralls @mpsonntag