Proposal (and bugfix) to implement value-based conditional requirement

[phargogh]

This proposal implements value-based conditional requirement expressions in validation.

The use case for this is that sometimes (like in UNA, see #1509 ), validation is conditional not only on whether a parameter is provided and has a value (is "sufficient"), but sometimes validation depends on the value of another key. In the case of UNA, several different inputs are required depending on the value of search_radius_mode. If search_radius_mode represents uniform search radii, then args['search_radius'] is required. If search_radius_mode represents search radii by population group, then args['population_gorup_radii_table'] is required.

An alternate approach to this could have been to just write out the required logic in UNA's validate function, but this feature also seemed useful and perhaps worth expanding. I would be happy to replace this implementation with such a model-specific implementation if that is preferred.

Implementation-wise, I thought it might be convenient to be able to use a dot-notation in the MODEL_SPEC expression to represent the argument's value. So for the above example, one might use search_radius_mode.value == "{RADIUS_OPT_UNIFORM}". Behind the scenes, validation (as proposed) will tokenize the expression and rewrite search_radius_mode.value into a new local variable name. This allows backwards compatibility with existing expressions throughout InVEST, while supporting the proposed dot-notation.

Critical feedback is very welcome!

Fixes #1509
Fixes #1165

Checklist

• Updated HISTORY.rst and link to any relevant issue (if these changes are user-facing)
  - [ ] Updated the user's guide (if needed)
• Tested the Workbench UI (if relevant)

[emlys]

@phargogh I really like where this is going! Our existing validation doesn't yet evaluate conditional requirement of CSV columns or vector fields, so I think that would need to be added. I think we just left it out of the original ARGS_SPEC design to keep things simple, but it would be a great improvement to have.

I found two other models where I left in a placeholder conditional requrirement for CSV columns. It would be great to have those working too:

invest/src/natcap/invest/forest_carbon_edge_effect.py

Lines 83 to 106 in 9e50106

	"c_below": {
	"type": "number",
	"units": u.metric_ton/u.hectare,
	"required": "pools_to_calculate == 'all'",
	"about": gettext(
	"Carbon density value for the belowground carbon "
	"pool. Required if calculating all pools.")
	},
	"c_soil": {
	"type": "number",
	"units": u.metric_ton/u.hectare,
	"required": "pools_to_calculate == 'all'",
	"about": gettext(
	"Carbon density value for the soil carbon pool. "
	"Required if calculating all pools.")
	},
	"c_dead": {
	"type": "number",
	"units": u.metric_ton/u.hectare,
	"required": "pools_to_calculate == 'all'",
	"about": gettext(
	"Carbon density value for the dead matter carbon "
	"pool. Required if calculating all pools.")
	},

invest/src/natcap/invest/urban_cooling_model.py

Lines 72 to 96 in 9e50106

	"shade": {
	"type": "ratio",
	"required": "cc_method == factors",
	"about": gettext(
	"The proportion of area in this LULC class that is "
	"covered by tree canopy at least 2 meters high. "
	"Required if the 'factors' option is selected for "
	"the Cooling Capacity Calculation Method.")},
	"albedo": {
	"type": "ratio",
	"required": "cc_method == factors",
	"about": gettext(
	"The proportion of solar radiation that is directly "
	"reflected by this LULC class. Required if the "
	"'factors' option is selected for the Cooling "
	"Capacity Calculation Method.")},
	"building_intensity": {
	"type": "ratio",
	"required": "cc_method == intensity",
	"about": gettext(
	"The ratio of building floor area to footprint "
	"area, with all values in this column normalized "
	"between 0 and 1. Required if the 'intensity' option "
	"is selected for the Cooling Capacity Calculation "
	"Method.")}

It's interesting that we've done this before in a limited way with boolean inputs by lumping in their true/false value with "input sufficiency". Which has worked well, but would it make more sense to explicitly check the value of the input like you propose here? For instance,

• "required": "do_valuation.value == True" vs
• "required": "do_valuation.value" vs
• "required": "do_valuation"?

[phargogh]

Thanks so much for taking a look at this @emlys ! I made a few changes that ultimately resulted in a larger refactor that really simplified validate. Here's a summary:

Expressions Now Use Arg Values

Symbols in expressions that represent args keys will now contain the user-provided value of the arg. If the key is not present in args, a value of False is assumed in these expressions. The symbol map passed to the expression is therefore {key: args.get(key, False) for key in spec}.

Even though we have been assuming so far that keys in expressions represent sufficiency (whether a key has a non-empty value in args), sufficiency is really just the truthy evaluation of the arg value ... so why not just use the arg value itself? There was only one place in all of InVEST where the expression needed to be changed (in UCM) because of this change in meaning.

This also gets around your question about the value or sufficiency of checkboxes. By switching to using the values of the checkboxes, we're being more explicit about just using the value of the input (whatever type that is), and allowing the programmer to decide how to handle its type in an expression that evaluates to a bool.

Expression Results are Cast to bool

Now that expressions use arg values, the result of the eval'd expression is now explicitly cast to bool. This allows us to ensure that we have only 2 possible states for conditional validity: valid and invalid. This works because a string with nonzero length is interpreted as True in a truthy context.

Switching to Truthiness = No Need to tokenize

Switching to truthiness means that there's no need to do any special parsing or mangling of variable names - we've simplified the meaning of the expression to be closer to how python handles expressions.

[emlys]

Thanks @phargogh! I really like how this simplifies validation. I had some questions but nothing major.

[emlys]

Would an arg value of 0 be labeled insufficient here?

[phargogh]

This specific line has been removed in a prior edit, but this is a case that the authors of expressions will need to be aware of. One cost of this proposed simplification is that it is possible for there to be a falsey value (like 0) which was previously acknowledged as "sufficient" to now be effectively insufficient. This can be mitigated by better, context-aware expressions in cases where this may be an issue, so I think we'll largely be OK. In practice, I don't think there are many places where we are likely to encounter a value of 0.

[emlys]

If I'm understanding, there is a subtle change here: before, we removed excluded_keys from the set to validate. Now, any keys that have a value will be validated, even if they're not required. I'm not sure what the correct answer is, but it seems worth noting the change.

[phargogh]

Yes, this exclusion was definitely something I needed to address, and the fix now includes a better variable name for excluded_keys to clarify how they were being used.

[emlys]

@phargogh might there still be a difference if a non-required key has a real value? Suppose you have a valuation_table that's required only if do_valuation is True. Then if we provide args where do_valuation is False and valuation_table is a valid path, should we still validate valuation_table?

[phargogh]

Great question, and thank you for thinking critically about this!

This specific snippet we're discussing here isn't updating for some reason, but there is now logic to cover the case where a non-required parameter has a truthy value. So yes, in the latest revision (at least as of rev 617e76d) I would expect the validation you describe to take place.

[emlys]

Ah okay, thanks for clarifying. I think that validation would not take place before, because excluded_keys were removed from the set to validate:

invest/src/natcap/invest/validation.py

Lines 1003 to 1004 in 9a70648

	sufficient_keys = set(args.keys()).difference(insufficient_keys)
	for key in sufficient_keys.difference(excluded_keys):

Is that a change worth noting in history?

[phargogh]

Oh! I see what you mean. Sorry for my confusion about this! Yes, this is indeed a change in behavior that I think is for the best and I have noted the changes in HISTORY. Once we figure out the code-signing issue, I'll share a dev build with Stacie to have her try it out.

[emlys]

@phargogh I just had a couple more comments. The test suite also failed - not sure if it's related

[phargogh]

Thanks @emlys ! Yes, the test failures were definitely related and I believe I have addressed everything. Let me know what you think!

[emlys]

Awesome, thanks @phargogh!