Coverage increased (+0.01%) to 78.918% when pulling b3a7cbd on nikhilwoodruff:parameter-scale-descendants into 30fc3db on openfisca:master.

OK I think I got everything:

• Unit test added and passed
• Versioning added (patch bump)
• Current tests and lint checks passing

@benjello @sandcha @MattiSG a review on this would be much appreciated, thanks!

Hello, Thank you for this fix! Just in case, did/could you test it on the Web API?

These, for example, should work: openfisca serve should launch the Web API without error and a GET to /parameter/{parameterID} should return the parameter information without error.

Tested locally on the Web API and:

• The command openfisca serve runs without error raised_hands
• Requesting the ParameterScale taxes.social_security_contribution works and gives the same result as before this PR raised_hands

  Request (and jq formatting): curl http://127.0.0.1:5000/parameter/taxes.social_security_contribution | jq

• But requesting all the parameters lists now scale elements as parameters which would be an issue for Web API users. For example, the list contains taxes.social_security_contribution[INDEX].threshold and taxes.social_security_contribution[INDEX].rate when, for this scale it should only contain taxes.social_security_contribution. Fix this issue before merging this PR?

  Request (and jq formatting): curl http://127.0.0.1:5000/parameters

And, later, if you both agree that this Web API issue is fixed, you can dismiss my review.

Thanks for this review, @sandcha. On your last point, I'm not sure if I follow: this PR intends to do exactly as you say. For example, in the UK we have a progressive income tax scale, but the rates have specific names ("basic rate", "higher rate", "additional rate"). We'd like to surface these parameters without having to take them outside of their parameter scale, using get_descendants. What do you think?

We'd like to surface these parameters without having to take them outside of their parameter scale

If I understand correctly, in the parameter scale, they would only be accessible through their numeric index. How would exposing them this way enable you to name them? 🤔

Alternative implementations

I understand the value in using a value both as a named one and as part of a parameter scale. The only way I know of this being implemented in the OF-FR corpus is by defining a legal constant (“plafond de la sécurité sociale”) as a parameter and expressing scales as multiples of it (threshold_unit: PSS). @openfisca/france-contrib do you know of other ways this has been implemented? Did you ever want to have such a feature?

Weight

The main issue I can see with this change is how much larger the /parameters route data would become. Currently, the aim of listing parameters is to make them discoverable, but not to expose all of their data and structure.

@sandcha since you ran this branch locally, could you please provide us with the file size increase for a large corpus such as OF-FR? As a baseline, curl -L -I https://api.fr.openfisca.org/latest/parameters | grep content-length yields 904421, i.e. 883 kB. What would be the added weight with this PR?

Thanks @MattiSG:

We'd like to surface these parameters without having to take them outside of their parameter scale

If I understand correctly, in the parameter scale, they would only be accessible through their numeric index. How would exposing them this way enable you to name them? thinking

Ah, I should mention we've added a name attribute to the parameter metadata for PolicyEngine, which overrides the default naming convention when it's served over the OpenFisca-US/UK API (here's the basic rate example).

Alternative implementations

I understand the value in using a value both as a named one and as part of a parameter scale. The only way I know of this being implemented in the OF-FR corpus is by defining a legal constant (“plafond de la sécurité sociale”) as a parameter and expressing scales as multiples of it (threshold_unit: PSS). @openfisca/france-contrib do you know of other ways this has been implemented? Did you ever want to have such a feature?

This is a very interesting idea I hadn't thought of. Would this be the first feature that enables Parameters to depend on other Parameters?

Weight

The main issue I can see with this change is how much larger the /parameters route data would become. Currently, the aim of listing parameters is to make them discoverable, but not to expose all of their data and structure.

@sandcha since you ran this branch locally, could you please provide us with the file size increase for a large corpus such as OF-FR? As a baseline, curl -L -I https://api.fr.openfisca.org/latest/parameters | grep content-length yields 904421, i.e. 883 kB. What would be the added weight with this PR?

Yes, this is a good point I hadn't considered.

Would this be the first feature that enables Parameters to depend on other Parameters?

In the example I gave, this “dependency” is only made clear through documentation and variables handle the (basic multiplication) maths. There is no explicit parameter linking that would be discoverable or handled by OF-Core.

Thanks for the additional information on name! This is much clearer now. But, on the other hand, the value of adding this to Core without this change becomes weaker 🙃 Note for future consolidation: this use case complements the proposals in openfisca/openfisca-france#1672 (comment).