search

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

Fix Enum projection bug #1086

Closed nikhilwoodruff closed 2 years ago

nikhilwoodruff commented 2 years ago

This PR fixes a bug where GroupPopulation.value_nth_person(EnumArray) -> Array[int], instead of the expected GroupPopulation.value_nth_person(EnumArray) -> EnumArray. I've extended the previous test case to cover this as well, to avoid code duplication.

coveralls commented 2 years ago

Coverage Status

Coverage decreased (-0.01%) to 78.908% when pulling 07fcfcfad6646e0eaaeba78154cacbae1555cbb3 on nikhilwoodruff:enum-projection into 49d95a08b8480b823a6e49b2ef16c8aeb8f0d196 on openfisca:master.

nikhilwoodruff commented 2 years ago

@benjello I added a test to cover this, should I add more? I think it covers the only use case.

benjello commented 2 years ago

@MattiSG @maukoquiroga we need an additional review here ;-)

nikhilwoodruff commented 2 years ago

Hey @sandcha @MattiSG @maukoquiroga - if you've got a spare second and this looks OK to add, would be much appreciated as we're having to use some workarounds in the US system (string type variable copies of affected Enums) that end up causing their own bugs. Thanks in advance!

sandcha commented 2 years ago

Seems ok. I will make a review of this PR tomorrow.

sandcha commented 2 years ago

@nikhilwoodruff From a "logistical" point of view, git rebase is favored in openfisca-core in order to get a linear history (easier to read etc.). Could you use git rebase instead of git merge?

You can for example compare their impacts by looking into the branch history with git log --pretty=format:'%h - %an : %s' --date-order --graph.

nikhilwoodruff commented 2 years ago

@nikhilwoodruff From a "logistical" point of view, git rebase is favored in openfisca-core in order to get a linear history (easier to read etc.). Could you use git rebase instead of git merge?

You can for example compare their impacts by looking into the branch history with git log --pretty=format:'%h - %an : %s' --date-order --graph.

Thanks @sandcha - I've updated the version number and changelog, so all the checks now pass. As for the rebasing, sounds good; I don't think I have permissions to merge the PR though?

nikhilwoodruff commented 2 years ago

Updating the versioning changes here after a recent PR was merged to ensure the versioning is still up-to-date here.

nikhilwoodruff commented 2 years ago

Thanks @benjello, @sandcha and @augusto-herrmann for the help with this PR!