openfisca / openfisca-core

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

Allow complete group encapsulation #1070

Closed nikhilwoodruff closed 3 years ago

nikhilwoodruff commented 3 years ago

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"].

nikhilwoodruff commented 3 years ago

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?

MattiSG commented 3 years ago

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 :wink:

bonjourmauko commented 3 years ago

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 #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 commented 3 years ago

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

MattiSG commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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:

MaxGhenis commented 3 years ago

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

MattiSG commented 3 years ago

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

Definitely 😉

nikhilwoodruff commented 3 years ago

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

Definitely wink

OK, added!

bonjourmauko commented 3 years ago

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

MattiSG commented 3 years ago

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 commented 3 years ago

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.

MattiSG commented 3 years ago

It may

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

bonjourmauko commented 3 years ago

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

nikhilwoodruff commented 3 years ago

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

MattiSG commented 3 years ago

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 😃