Allow complete group encapsulation

[nikhilwoodruff]

Hope this is useful - this fixes #1041 , by adding a shortcut to a containing group entity provided. I've added a test to test_formulas.

Essentially, say we have three entities, person, family and household, and we know that all members of a family will be in the same household (but not necessarily vice-versa). Say we also have a variable household_level_variable at the household level, and we want to project the result of that variable to the contained families. Currently, I think we have to use:

family_level_variable = family.value_from_first_person(family.members.household("household_level_variable", period))

With this, we can instead use:

family_level_variable = family.household("household_level_variable", period)

provided, that when we defined the family entity (using either build_entity or the Entity class), we set the optional keyword argument containing_entities = ["household"].

[MattiSG]

Thanks @nikhilwoodruff for this contribution! 😃 This seems to address the point you opened yourself in #1041.

• For value assessment: @MaxGhenis, you upvoted Allow complete group encapsulation #1041, can you confirm this implementation would solve your concern as well? 🙂
• For vision compatibility assessment, @benjello and @sandcha, could you please comment on this minor extension of the API surface? Does it seem idiomatic enough in OpenFisca? Do you see good uses in the tax and benefit systems you maintain? 🙂
• For technical implementation: I see a good test suite. @maukoquiroga, @benjello do you have any feedback on the implementation? 🙂

On my side as a gatekeeper, my main feedback is on semver and CHANGELOG, as outlined in the comments 🙂 I would also add that some documentation on this new feature would be an important addition, to enable its discoverability. @maukoquiroga, considering #1061, would you recommend to use doctests or to use openfisca/openfisca-doc?

As soon as the version number and link are corrected, if I'm too long to review again, it is okay for another maintainer to dismiss my review if it is the only blocking point.

[nikhilwoodruff]

Thanks @MattiSG - I've just committed your suggestions after increasing the version bump from 35.6.0 to 35.7.0 (after @benjello 's recent PR merge). Is there something I need to do to run the code coverage test?

Blocking comments have been addressed :)

[MattiSG]

Thanks @nikhilwoodruff for your fast reaction! 😃

As a side note, regarding the version bump and in order to save you effort in the future: due to the number of contributors and the asynchronous nature of contributions on OpenFisca, it is very common that version numbers conflict. In order to minimise the administrative burden, we tend to update the version number and rebase one last time after getting review approval, as otherwise the number of upstream merges can become a bit daunting 😉

[bonjourmauko]

Thanks @nikhilwoodruff for this contribution! 😃 This seems to address the point you opened yourself in #1041.

Fantastic work @nikhilwoodruff !

• For value assessment: @MaxGhenis, you upvoted Allow complete group encapsulation #1041, can you confirm this implementation would solve your concern as well? 🙂
• For vision compatibility assessment, @benjello and @sandcha, could you please comment on this minor extension of the API surface? Does it seem idiomatic enough in OpenFisca? Do you see good uses in the tax and benefit systems you maintain? 🙂
• For technical implementation: I see a good test suite. @maukoquiroga, @benjello do you have any feedback on the implementation? 🙂

The test is an integration one which seems to cover the use case, I've just added a couple of suggestions.

On my side as a gatekeeper, my main feedback is on semver and CHANGELOG, as outlined in the comments 🙂 I would also add that some documentation on this new feature would be an important addition, to enable its discoverability. @maukoquiroga, considering #1061, would you recommend to use doctests or to use openfisca/openfisca-doc?

I do recommend at least documenting the test itself, and the argument, I've made an example proposal.

Other than that, for a real-life use-case scenario, I'd rather see it in openfisca/openfisca-doc —I hope both get merged soon, but for now, there :)

As soon as the version number and link are corrected, if I'm too long to review again, it is okay for another maintainer to dismiss my review if it is the only blocking point.

[MattiSG]

Thank you so much @maukoquiroga for your fast reaction and these very detailed suggestions 🤩

[MattiSG]

Is there something I need to do to run the code coverage test?

No, unfortunately it seems our CI config is not properly set up to handle incoming PRs from external organisations. The token needed to pass the CI coverage step is not passed to such PRs for security reasons, but the step is still run and thus fails instead of being cancelled. This is something to be fixed in #1057.

[benjello]

@nikhilwoodruff @maukoquiroga @MattiSG : I am ok with the idea of introducing encapsulated entities.
My suggestion is to add some checks of proper encapsulation at the Population data injection step.

[bonjourmauko]

@benjello You mean when the SimulationBuilder creates the GroupEntity? It is still a bit of a fuzzy process for me, so a pointer would be much welcomed :)

[MaxGhenis]

For value assessment: @MaxGhenis, you upvoted #1041, can you confirm this implementation would solve your concern as well? 🙂

Yes, this is great, thanks @nikhilwoodruff. Just wanted to check if this works for enums? I don't see one in the test.

[nikhilwoodruff]

For value assessment: @MaxGhenis, you upvoted #1041, can you confirm this implementation would solve your concern as well? slightly_smiling_face

Yes, this is great, thanks @nikhilwoodruff. Just wanted to check if this works for enums? I don't see one in the test.

Just tried to get a MWE (I know we've had an issue in the past where projection loses the Enum decodability), but found it does work! Here's a MWE - I tested:

• person Enum -> household Enum
• household Enum -> person Enum
• household Enum -> family Enum (with this PR)

[MaxGhenis]

Nice, would those enum examples make sense to include as tests here?

[MattiSG]

would those enum examples make sense to include as tests here?

Definitely 😉

[nikhilwoodruff]

would those enum examples make sense to include as tests here?

Definitely wink

OK, added!

[bonjourmauko]

Great ✨ !!! As soon as we deal with #1073 that failing check should go away.

[MattiSG]

Coverage tests are not required, we should not wait for #1073 to merge this 🙂
I wonder however if test_docs should not be made mandatory, wdyt @maukoquiroga? If this was merged as is, would it break the public documentation?

[bonjourmauko]

Oh @MattiSG my mistake, I though coverage and test-docs were required.

Coverage tests are not required, we should not wait for #1073 to merge this 🙂

Definitely.

If this was merged as is, would it break the public documentation?

It may, but anyway this needs to rebased before merging, so I'd rather fix that.

[nikhilwoodruff]

Thanks @maukoquiroga for this fix for the doc tests - it seems that the code linter and the documentation parser have a contradictory requirement here? I'm having issues installing the dependencies for make test-doc so I'm just running it on the GH actions (this last fix fixed the doc test, but broke the code lint check).

[bonjourmauko]

Yes I saw that 😢

But @nikhilwoodruff you may just leave it as it is, either @MattiSG or I can fix this while rebasing for merging.

(Which we'll do sooner rather than later) 😃

[bonjourmauko]

Still, I agree this is at best confusing, I wonder why there's a discrepancy.

[bonjourmauko]

If you are using the Google Python Style you will sometimes get unwanted warnings from this plugin - particularly in the argument descriptions - as it does not use strict RST. We therefore currently suggest ignoring some of the violation codes:

    [flake8]
    extend-ignore =
        # Google Python style is not RST until after processed by Napoleon
        # See https://github.com/peterjc/flake8-rst-docstrings/issues/17
        RST201,RST203,RST301,

https://pypi.org/project/flake8-rst-docstrings/

[MattiSG]

It may

Then I've just made test_docs mandatory. We don't want to break doc in production 😅

[bonjourmauko]

@nikhilwoodruff I think we can just ignore the failing check, I'm rebasing and LGTM ✨ 💯 !

[nikhilwoodruff]

Amazing! Thanks @maukoquiroga , @MattiSG and @benjello for the reviews and comments, great to finally make a contribution to Core.

[MattiSG]

Thanks @nikhilwoodruff for your reactivity and openness to feedback, it's great to have you as a new contributor! 😊
Thanks to everyone who chimed in and evolved this initial contribution into this well-tested, documented feature 💞
And of course thanks @maukoquiroga for your detailed technical feedback and handling the final steps. It is amazing how many side discoveries we had with this PR, from fixing coverage collection to actually running doctests to finding mutually exclusive linting conditions 😃