openfisca / openfisca-core

OpenFisca core engine. See other repositories for countries-specific code & data.
https://openfisca.org
GNU Affero General Public License v3.0
165 stars 74 forks source link

Support `str` and/or `date` parameters #1092

Open nikhilwoodruff opened 2 years ago

nikhilwoodruff commented 2 years ago

Is there a technical reason why OpenFisca-Core doesn't allow string or date types in parameters? When I try to add one, it fails the validation check in the parameter initialisation. Some legislative parameters are date-based - e.g. pension changes often affect only those born after a specific date. Happy to file a PR adding this if it's not objectionable.

benjello commented 2 years ago

I do not recall an obvious reason.

For parameters changing on another dimension than the legislation time (period), and specifically parameters depending on a date (like the date of birth for pension modelling), I started something ugly in #978 but worth looking at.

cc @eraviart @MattiSG @maukoquiroga

nikhilwoodruff commented 2 years ago

Thanks @benjello - that PR is very cool!

benjello commented 2 years ago

@nikhilwoodruff : IIRC there was also a problem with starting a parameter name with a number since it will be somehow an attribute (to allow the use of the dotted noation of the parameters) and an attribute name cannot start wit a number?

nikhilwoodruff commented 2 years ago

Interesting @benjello - though hasn't OpenFisca already made an made an exception for this via the ParameterScale?

benjello commented 2 years ago

@nikhilwoodruff : could you provide an example. My remark points to the fact that I used ne_avant_YYYY_MM_DD and ne_apres_YYYY_MM_DD in #978 because I vaguely recall that I was not able to use YYYY_MM_DD as Parameter or a ParameterNode.

nikhilwoodruff commented 2 years ago

Sure, I meant how e.g. in OpenFisca-UK the second rate of income tax has the name tax.income_tax.rates.uk[1].rate due to it being part of a parameter scale.

benjello commented 2 years ago

It could have been more convenient to have : tax.income_tax.rates.uk.1.rate but it is not possible for the reason I mentioned IIRC.

MattiSG commented 2 years ago

I do not find a good documented reason for not supporting String or Date types in parameters. The test mentioned in #1094 was added in #552, which switched parameter types to YAML. I might very well have asked that we test these types just because they were absent in the original parameter base and it was a good smoke test to make sure we did not add any.

However, I am a bit concerned with adding such a feature lightly. Extending parameters to a whole new class seems likely to yield many side effects: all handlers (Web API, loaders, tests…) have to be fully tested for support of that new type.

@nikhilwoodruff could you please document here clearly two use cases for such types? 🙂 From what I understand, the main one would be to store dates, am I correct? If that is the case, could you please provide us with the legislation you want to model and how you would like to do it? Is there any other?

I understand this might be frustrating, but there has been a lot of work in evolving the parameter declaration developer experience, and I believe it is important we keep it light and consistent 🙂

nikhilwoodruff commented 2 years ago

Thanks for the clarification @MattiSG - OK, so admittedly the date usage I originally had in mind is not a legislative parameter (part of a current-date feature of PolicyEngine) and so I don't see that being helpful in justifying this. But what about these, which would be an improvement to represent closer to how they are specified in legislation for the UK:

nikhilwoodruff commented 2 years ago

Coming back to this, it might be relevant to consider how the current parameter allowed types include lists (including lists of strings). I think there are very clear legislative use-cases for this: for example, income definition lists for means tests or taxes.

On the UK and US models, given the legislative examples above, I think we'll use the list parameter as an intermediate workaround for string parameter types (using a list of length 1, containing a single string). But this strikes me as less ideal: OpenFisca-Core does support string parameters (and therefore already has the side effects you mention, if I understand this correctly), but just without explicitly documented support.

@MattiSG does this make sense? Happy to expand on the examples above too if needed.

MattiSG commented 2 years ago

Thanks @nikhilwoodruff!

Strings

I agree that lists of strings are currently used and useful. Example use cases:

I am thus not sure to understand the difference between “supporting strings” and these examples. Could you please give a code example of how you would implement the “UK country for each county” in a way that is not currently supported by OF-Core? 🙂 You can use the section below as an example of how to present it.

Dates

As mentioned earlier, I don't see any good reason not to support dates. Am I correct that, in the Child Tax Credit act you linked to, the implementation you'd like to have would be something like:

child_tax_credit:
  born_before:
    metadata:
      unit: date
    values:
      2016-03-16:
        value: 2017-04-06

How would you see this exposed in Python and JSON? Should the Python API expose a Numpy Datetime object (i.e. hydrate the parameter to an np.datetime64)? Should the JSON (“web”) API expose an ISO 8601 date (i.e. "2017-04-06")? 🙂

MattiSG commented 2 years ago

I am very surprised that dates are never used in the OF-FR parameters corpus. I did look for value: \d{4}- and there are indeed no results. @benjello do we really have no such modelled case where the French legislation provides a date?!

benjello commented 2 years ago

@MattiSG : I do not recall such examples but we can actually use dates. For example, there are some family allowance that are dismantled and only the kids born before a specific date can apply.

MattiSG commented 2 years ago

Thanks @benjello!

Just to be 100% sure: when you say “we can actually use dates”, you mean that the French law can do it, right? Not that OpenFisca-Core can currently do it, or that OpenFisca-France already uses it? 😅

benjello commented 2 years ago

Yes @MattiSG we could benefit from such a feature but we do not use it right now in our implementation IIRC.