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

Support str and/or date parameters #1092

Open
nikhilwoodruff opened this issue Dec 3, 2021 · 15 comments · May be fixed by #1094
Open

Support str and/or date parameters #1092

nikhilwoodruff opened this issue Dec 3, 2021 · 15 comments · May be fixed by #1094

Comments

@nikhilwoodruff
Copy link
Contributor

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
Copy link
Member

benjello commented Dec 3, 2021

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
Copy link
Contributor Author

Thanks @benjello - that PR is very cool!

@benjello
Copy link
Member

benjello commented Dec 4, 2021

@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
Copy link
Contributor Author

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

@benjello
Copy link
Member

benjello commented Dec 4, 2021

@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
Copy link
Contributor Author

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
Copy link
Member

benjello commented Dec 4, 2021

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.

@nikhilwoodruff nikhilwoodruff linked a pull request Dec 10, 2021 that will close this issue
@MattiSG
Copy link
Member

MattiSG commented Dec 10, 2021

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
Copy link
Contributor Author

nikhilwoodruff commented Dec 10, 2021

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
Copy link
Contributor Author

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
Copy link
Member

MattiSG commented May 5, 2022

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
Copy link
Member

MattiSG commented May 5, 2022

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
Copy link
Member

benjello commented May 5, 2022

@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
Copy link
Member

MattiSG commented May 5, 2022

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
Copy link
Member

benjello commented May 5, 2022

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

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 a pull request may close this issue.

3 participants