Skip to content

Conversation

MattiSG
Copy link
Member

@MattiSG MattiSG commented Mar 17, 2021

Technical changes

  • Support arrays as parameters.
  • Improves wrong parameter types error message.

This was added to move more constants into parameters. For example, in OpenFisca-France: https://github.com/openfisca/openfisca-france/blob/master/openfisca_france/model/caracteristiques_socio_demographiques/demographie.py#L3-L35


TODO :

  • Add Web API tests.
  • Update documentation which currently states that parameters are “numeric parameters”.

@MattiSG
Copy link
Member Author

MattiSG commented Mar 17, 2021

CI should be fixed by #981.

@MattiSG
Copy link
Member Author

MattiSG commented Mar 22, 2021

Output from web-api should be checked before merging.

@MattiSG MattiSG force-pushed the allow-array-parameters branch from f6172d9 to 4e66961 Compare March 23, 2021 16:36
@MattiSG
Copy link
Member Author

MattiSG commented Mar 23, 2021

@benjello can you please confirm this should not damage vectorial computation performance?

@MattiSG
Copy link
Member Author

MattiSG commented Mar 23, 2021

I wrote the associated test in a manner decoupled from the country template, in order to avoid a repeat from #980 and in prevision of #984. However, that means discoverability from this feature will be limited in the country template.

@MattiSG
Copy link
Member Author

MattiSG commented Mar 23, 2021

Documentation update can be reviewed at openfisca/openfisca-doc#234.

@benjello
Copy link
Member

benjello commented Mar 23, 2021

@benjello can you please confirm this should not damage vectorial computation performance?

I do not see how this could damage vectorial computation. LGTM.

@MattiSG
Copy link
Member Author

MattiSG commented Mar 23, 2021

API output tested manually on a fork of the country template, got the following result:

{
  "description": "Array types should be allowed.",
  "id": "general.array_type",
  "metadata": {},
  "source": "https://github.com/openfisca/country-template/blob/3.12.4/openfisca_country_template/parameters/general/array_type.yaml",
  "values": {
    "2018-06-01": ["ES", "FR", "IT", "NZ"],
    "2013-01-01": ["FR"]
  }
}

Not sure about how the legislation explorer will format these results, though.

# It is now recommended to include them in metadata, until a common consensus emerges.
COMMON_KEYS = {'description', 'metadata', 'unit', 'reference', 'documentation'}
ALLOWED_PARAM_TYPES = (float, int, bool, type(None))
ALLOWED_PARAM_TYPES = (float, int, bool, type(None), typing.List)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any preference between typing.List and just List? Asking just for consistency (current usage is from typing import XXX, but that is not consistent with other libraries usage, for example numpy and so on).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No preference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use from typing import List just for consistency? Otherwise GTM :)


def test_array_type():
path = os.path.join(BASE_DIR, 'array_type.yaml')
load_parameter_file(path, 'array_type')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
load_parameter_file(path, 'array_type')
assert load_parameter_file(path, 'array_type')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will anyway pass as it is a truthy value -otherwise it would fail. Just a proposition for reader's clarity.

Copy link
Member

@bonjourmauko bonjourmauko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution! Looks good to me ✨

MattiSG and others added 2 commits March 23, 2021 17:46
Co-authored-by: Mauko Quiroga <[email protected]>
Co-authored-by: Mauko Quiroga <[email protected]>
@MattiSG MattiSG force-pushed the allow-array-parameters branch from f646d36 to 77d5808 Compare March 23, 2021 17:46
@MattiSG MattiSG merged commit cd8e6f3 into master Mar 23, 2021
@MattiSG MattiSG deleted the allow-array-parameters branch March 23, 2021 17:55
@bonjourmauko bonjourmauko added the kind:feat A feature request, a feature deprecation label Mar 26, 2021
@MattiSG
Copy link
Member Author

MattiSG commented Mar 26, 2021

@maukoquiroga @benjello you approved this PR —thanks again! 🙂 Would you mind spending 3 minutes reviewing the associated doc update? It's really short and would avoid an outdated documentation to create confusion: openfisca/openfisca-doc#234
Thanks! 🙏

@bonjourmauko
Copy link
Member

Thanks @MattiSG and @benjello for the review :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:feat A feature request, a feature deprecation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants