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

Enable single rule disjunction #1369

Closed
wants to merge 124 commits into from

Conversation

GiaJordan
Copy link
Contributor

@GiaJordan GiaJordan commented Feb 13, 2024

Changelog

This PR enables single rule disjunction for the existing validation rules through the IsNA rule modifier. It can be used with each rule except the recommended rule by including it in the specification for validation rules as another rule (ie with the :: operator).

When in use, Validation rules will raise an error or warning or no message as appropriate for each metadata entry that is provided, and will not raise any message for metadata entries that are omitted. The attribute should be marked as FALSE for the REQUIRED column in the data model to use this modifier.

A data model specific to these requirements has been created to monitor the status of each rule combined with this modifier and tests have been created as well.


Note for Reviewers:

The line endings for some files were changed and so the diffs can show entire file changes as part of this PR. Before you begin reviewing, under Files changed>Settings toggle Hide Whitespace to ON to get an accurate record of what actual code was changed.
image

@GiaJordan GiaJordan marked this pull request as draft February 13, 2024 16:56
@GiaJordan GiaJordan changed the title Develop single rule disjunction fds 1352 Enable protectAges or inRange to be modified with IsNA Feb 13, 2024
@GiaJordan GiaJordan changed the title Enable protectAges or inRange to be modified with IsNA Enable protectAges and inRange rules to be modified with IsNA Feb 14, 2024
@GiaJordan GiaJordan changed the title Enable protectAges and inRange rules to be modified with IsNA Enable single rule disjunction Feb 14, 2024
@GiaJordan
Copy link
Contributor Author

GiaJordan commented Feb 15, 2024

@andrewelamb thank you for your comments but this PR was not ready for review; it is still a work in progress. #1366 was what I presented in code review yesterday; this is the one I said I would tag you and @/mialy-defelice in when it was complete and ready to be reviewed

@andrewelamb
Copy link
Contributor

Oops! Sorry about that, you are right.

@GiaJordan GiaJordan force-pushed the develop-single-rule-disjunction-FDS-1352 branch from 9128b28 to ba914ea Compare February 15, 2024 18:09
@GiaJordan GiaJordan force-pushed the develop-single-rule-disjunction-FDS-1352 branch from 7164415 to dcd8a68 Compare February 19, 2024 18:00
@GiaJordan GiaJordan force-pushed the develop-single-rule-disjunction-FDS-1352 branch from 3d8a8f2 to 6c7e2a4 Compare March 7, 2024 22:21
yield dmge

@pytest.fixture(name="missing_dmge")
def missingDMGE(helpers):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is minor but can you change this naming to something more descriptive? The way it reads is a bit confusing.
Maybe something like missing_value_model_DMGE

yield metadataModel

@pytest.fixture
def missingMetadataModel(helpers):
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above but in this case something like: missing_value_model_MetadataModel

@mialy-defelice
Copy link
Contributor

mialy-defelice commented Mar 8, 2024

@GiaJordan
When running the following (also happens without -rr):

schematic model -c config.yml validate -mp tests/data/mock_manifests/Invalid_Missing_Value_Test_Manifest.csv -dt MockComponent -rr

With the model:
"tests/data/example.missing.value.model.csv"
I got the following error:

['NA', 'Check Regex List', "For attribute Check Regex List, the provided validation rules (['list strict error', 'regex match [a-f] error', 'IsNa']) .have too many entries. We currently only specify two rules ('list :: another_rule').",

The multiple types error handling should be updated to allow for a rule combo to be paired with isNA.

@mialy-defelice
Copy link
Contributor

Also if you run:

schematic model -c config.yml validate -mp tests/data/mock_manifests/Invalid_Missing_Value_Test_Manifest.csv -dt MockComponent -rr

with the example.model.jsonld
It throws this error:

File "/Users/mialydefelice/Library/Caches/pypoetry/virtualenvs/schematicpy-rs9bwhSl-py3.9/lib/python3.9/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
  File "/Users/mialydefelice/Library/Caches/pypoetry/virtualenvs/schematicpy-rs9bwhSl-py3.9/lib/python3.9/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
  File "/Users/mialydefelice/Library/Caches/pypoetry/virtualenvs/schematicpy-rs9bwhSl-py3.9/lib/python3.9/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/mialydefelice/Library/Caches/pypoetry/virtualenvs/schematicpy-rs9bwhSl-py3.9/lib/python3.9/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/mialydefelice/Library/Caches/pypoetry/virtualenvs/schematicpy-rs9bwhSl-py3.9/lib/python3.9/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/mialydefelice/Library/Caches/pypoetry/virtualenvs/schematicpy-rs9bwhSl-py3.9/lib/python3.9/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
  File "/Users/mialydefelice/Library/Caches/pypoetry/virtualenvs/schematicpy-rs9bwhSl-py3.9/lib/python3.9/site-packages/click/decorators.py", line 45, in new_func
    return f(get_current_context().obj, *args, **kwargs)
  File "/Users/mialydefelice/Documents/schematic_e/schematic/schematic/models/commands.py", line 263, in validate_manifest
    errors, warnings = metadata_model.validateModelManifest(
  File "/Users/mialydefelice/Documents/schematic_e/schematic/schematic/models/metadata.py", line 275, in validateModelManifest
    errors, warnings, manifest = validate_all(
  File "/Users/mialydefelice/Documents/schematic_e/schematic/schematic/models/validate_manifest.py", line 317, in validate_all
    manifest, vmr_errors, vmr_warnings = vm.validate_manifest_rules(
  File "/Users/mialydefelice/Documents/schematic_e/schematic/schematic/models/validate_manifest.py", line 238, in validate_manifest_rules
    vr_errors, vr_warnings = validation_method(
  File "/Users/mialydefelice/Documents/schematic_e/schematic/schematic/models/validate_attribute.py", line 765, in type_validation
    if bool(value) and not isinstance(value, specified_type[val_rule]):
  File "missing.pyx", line 392, in pandas._libs.missing.NAType.__bool__
TypeError: boolean value of NA is ambiguous

Im guessing that in TestManifestValidation It should run in combo, testing the invalid_missing_manifest against the standard example model. In this case you can check for expected errors. Should run in other combos as well to ensure coverage.

val_rule.__contains__("matchAtLeastOne")
and len(present_manifest_log) < 1
):
aggregation_functions = {
Copy link
Contributor

Choose a reason for hiding this comment

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

For function clarity can you move this into its own function?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will deal with these functions when this branch merges with the matchNone branch.

Base automatically changed from develop-val-error-handling-FDS-1352 to develop March 21, 2024 16:58
@mialy-defelice
Copy link
Contributor

@GiaJordan why is ths not being merged into develop?

This will now be merged directly to dev.

@mialy-defelice
Copy link
Contributor

Also if you run:

schematic model -c config.yml validate -mp tests/data/mock_manifests/Invalid_Missing_Value_Test_Manifest.csv -dt MockComponent -rr

...

Fixed this issue.

@mialy-defelice
Copy link
Contributor

@GiaJordan When running the following (also happens without -rr):

schematic model -c config.yml validate -mp tests/data/mock_manifests/Invalid_Missing_Value_Test_Manifest.csv -dt MockComponent -rr

With the model: "tests/data/example.missing.value.model.csv" I got the following error:

['NA', 'Check Regex List', "For attribute Check Regex List, the provided validation rules (['list strict error', 'regex match [a-f] error', 'IsNa']) .have too many entries. We currently only specify two rules ('list :: another_rule').",

The multiple types error handling should be updated to allow for a rule combo to be paired with isNA.

Okay, looking through the tests it looks like this is expected, but Its unclear why... need to investigate further...

@mialy-defelice
Copy link
Contributor

Okay, looking through the tests it looks like this is expected, but Its unclear why... need to investigate further...

Okay, I think this is bc the error was anticipated but decided that it was within the acceptance criteria. We are going to allow NA to be used with other rule combos, so for now will need to adjust the number of allowable rules, update testing to no longer allow this error to be considered passing.

Copy link

Quality Gate Passed Quality Gate passed

Issues
75 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@mialy-defelice mialy-defelice deleted the develop-single-rule-disjunction-FDS-1352 branch April 17, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants