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

Portfolio validation fails if limit and deductible type columns are missing #57

Open
carlfischerjba opened this issue Sep 5, 2023 · 6 comments
Assignees
Labels
question Further information is requested

Comments

@carlfischerjba
Copy link

Issue Description

The limit and deductible type fields have a default value of 0 (treat as an absolute amount) and are therefore not mandatory. However, portfolio validation fails if columns such as LocDedType3Contents or LocLimitType1Building are missing. By adding in empty columns, validation passes. Empty column or absent column should be treated the same.

Steps to Reproduce (Bugs only)

  1. Run ods_tools check --location exposure_loc_latlon_TCs.csv (attached).
  2. See errors about missing columns.

Expected behaviour: no errors and the default value from the spec is used.

(The error about missing LocPeril is a separate issue. It would be convenient if the default was to apply to all perils, or possibly use the value of LocPerilsCovered but I admit I'm unclear about the purpose of this field.)

Version / Environment information

ods_tools 3.1.0

Example data / logs

exposure_loc_latlon_TCs.csv

@benhayes21
Copy link
Contributor

@carlfischerjba - these are conditionally required fields, so the do need to be there is the parent field is present.

@benhayes21 benhayes21 self-assigned this Oct 18, 2023
@carlfischerjba
Copy link
Author

@carlfischerjba - these are conditionally required fields, so the do need to be there is the parent field is present.

@benhayes21 what's the parent field in this case?

But that doesn't explain why we need to supply an empty column. If a value is required, it would make sense to require a column with values but an empty column does nothing to provide those values. If the software is able to use a default value when the column is empty, it should be able to use the same default value when the column is missing.

@johcarter
Copy link
Contributor

Hi @carlfischerjba its more of a sibling than a parent. LocDed{} and LocDedType{} fields are mutually required, and its the same across all hierarchal levels.

Its important to include the type fields as a common error is to forget about it and not identify % terms as a type 1 or 2. Fractional numbers get applied as monetary fields, and then the numbers are incorrect and nobody realises.

I do see your point though. I don't know what default value would be best to avoid the above situation. Any thoughts?

@carlfischerjba
Copy link
Author

Its important to include the type fields as a common error is to forget about it and not identify % terms as a type 1 or 2. Fractional numbers get applied as monetary fields, and then the numbers are incorrect and nobody realises.

I do see your point though. I don't know what default value would be best to avoid the above situation. Any thoughts?

If there is no sensible default then shouldn't an empty column be rejected in the same way as a missing column? We're not providing any extra information when adding the extra empty column. Seems like it's an issue with how the validation handles missing values and missing columns rather than a problem with the schema itself (which I'm not well placed to judge).

@benhayes21
Copy link
Contributor

The validation just uses the schema definitions though. Ideally the validation will be dumb and just accept what's in the schema - we don't really want it to make its own decisions outside of the schema.

For DedType fields, the schema has CR as required (i.e. conditional requirement), Blanks Allowed and a default of 0. It might be as you say that 0 is not a sensible default and we should force the field to be consciously populated or else reject the file in the validation. Or we could not allow blanks to force the same behaviour.

A third option might be to implement some more complex default logic - e.g. default to 0 if (ded > 1) or 1 if (0 < ded <=1)

But whatever we do, I'm keen on the schema being the thing that drives the rules and the validation tool applies the schema.

@benhayes21 benhayes21 added the question Further information is requested label Oct 19, 2023
@carlfischerjba
Copy link
Author

There are a couple of different things going on.

  1. Could we see this as a software issue rather than a schema issue? If a field is marked as required (or conditionally required) the column must be present and populated. Currently, the validation accepts an empty column as a value even though no value is provided (since the column is empty). Or the reverse, where we see that a column is present but empty (or present but containing default values), treat it as if it was not present and do not require any extra conditional columns.
  2. In the specific case where limits and deductibles are zero, there's no value in saying how they should be interpreted since an absolute value of zero is the same as zero percent of TIV which is the same as zero percent of loss.

There's no need to change the schema, just how the schema is interpreted and enforced by the software.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
Status: Todo
Development

No branches or pull requests

3 participants