Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't force persons to be assigned to all defined entities #667

Closed
jacob88 opened this issue May 19, 2018 · 12 comments
Closed

Don't force persons to be assigned to all defined entities #667

jacob88 opened this issue May 19, 2018 · 12 comments
Assignees
Labels
kind:feat A feature request, a feature deprecation

Comments

@jacob88
Copy link

jacob88 commented May 19, 2018

Hi there!

I really enjoy OpenFisca, but I recently encountered an issue.

Here is what I did:

Declared "Bob" in Persons without assigning "Bob" to a titled_property,
through the JSON web API.

{
	"persons": {
		"Bob": {
			"sum_of_earnings_in_last_52_weeks": {
				"2018": 1200
			},
			"earnings_period_in_weeks": {
				"2018": 52
			},
			"weekly_compensation_before_tax": {
				"2018": null
			}
		}
	}
}

Here is what I expected to happen:

I expected the API to return "200 OK" with the null field filled with the result

{
	"persons": {
		"Bob": {
			"earnings_period_in_weeks": {
				"2018": 52
			},
			"sum_of_earnings_in_last_52_weeks": {
				"2018": 1200
			},
			"weekly_compensation_before_tax": {
				"2018": 23.076923370361328
			}
		}
	}
}

Here is what actually happened:

{
  "titled_properties": "Bob has been declared in persons, but is not a member of any titled_property. All persons must be allocated to a titled_property."
}

Here is data (or links to it) that can help you reproduce this issue:

Required an additional titled property.

Below works fine.

{
	"persons": {
		"Bob": {
			"sum_of_earnings_in_last_52_weeks": {
				"2018": 1200
			},
			"earnings_period_in_weeks": {
				"2018": 52
			},
			"weekly_compensation_before_tax": {
				"2018": null
			}
		}
	},
	"titled_properties": {
		"donors": {
			"owners": [
			"Bob"
			]
		}
	}
}

Context

I identify more as a:

  • Developer (I create tools that use the existing OpenFisca code).

(remove this line as well as all items in the list that don't fit your situation)

@MattiSG
Copy link
Member

MattiSG commented May 19, 2018

As an additional bit of context for other readers: this is in the context of https://github.com/ServiceInnovationLab/openfisca-aotearoa, with these entities.

@MattiSG
Copy link
Member

MattiSG commented May 19, 2018

Thank you so much @jacob88 for this detailed issue!

It looks indeed like all entities types that exist in a tax and benefit system have to be declared in a situation. I believe OpenFisca-France uses scenarios to overcome this limitation, making inferences and dynamically creating entities that have not been filled.
If that was right, funnily enough, there is an intent to remove scenarios in #540, and this current issue shows that there is a need for inferences anyway.

I'm wondering about the possibility of adding some metadata to entities declaration defining if they are optional, or how they could be inferred.

@MattiSG MattiSG changed the title All persons must be allocated to a titled_property. Persons have to be assigned to all defined entities in a tax and benefit system May 19, 2018
@fpagnoux
Copy link
Member

Thanks @jacob88 for reporting this!

Adding the additional titled property was indeed the way to solve that. I though understand the frustration of having to specify a titled property when you don't care about that.

It looks indeed like all entities types that exist in a tax and benefit system have to be declared in a situation.

Yep. This is the consequences of two principles:

  1. In OpenFisca Core, we currently have the following invariant: Every person belongs to one household (or any group entity), and only one
    • We can challenge this invariant, but that would be a deep change in the Core. There will be challenges due to the vectorial computation.
    • There seems to be a consensus that this is the right direction, but that will take time.
  2. The API has been voluntarily kept as low-level as possible. It therefore does not make inference, and wants you to fully describe the situation.

I believe OpenFisca-France uses scenarios to overcome this limitation, making inferences and dynamically creating entities that have not been filled.

Yes and no. It has been historically the case, and it is still for some use-cases (micro-simulation with data) but scenarios are not used in the new Web API. So there is no special treatment for France: users also have to specify all their entities (and that's worse, because France as 3 group entities).

I'm wondering about the possibility of adding some metadata to entities declaration defining if they are optional, or how they could be inferred.

Yep, functionally, that seems the way to go. But that's not clear who should be in charge of these inferences.

  • Should it be done by the Core module that creates a simulation based on the situation described by the user (relax principle 2)
  • Should it be done deeper in Core (change principle 1)

@MattiSG
Copy link
Member

MattiSG commented May 22, 2018

Awesome, thanks for the insights @fpagnoux! I feel we're making progress 👍

I suggest we keep accumulating use cases and challenges regarding relaxing principle 1 here until we feel we can create an RFC for changing it.
If really we can't figure out a way there we'll go with relaxing principle 2, but I feel the adequate mental model mapping is to not have to specify things that I don't know about (who has a titled property? who is in a family?) just like OpenFisca doesn't force me to define variables I don't know about 🙂

@benjello
Copy link
Member

benjello commented Jun 8, 2018

For a use case in favor of relaxing principle 1:

  • shared custody of child so he can belong tot two families or two fiscal units in France
  • one person can belong to two fiscal units if her spouse dies during that fiscal year

@benjello
Copy link
Member

benjello commented Jun 8, 2018

You can also use that to link an individual to several jobs

@fpagnoux
Copy link
Member

Input from @benjello about how this had been dealt with in a similar context: http://liam2.plan.be/documentation/0.12/links.html?highlight=links

@fpagnoux fpagnoux changed the title Persons have to be assigned to all defined entities in a tax and benefit system Don't force persons to be assigned to all defined entitiesd Aug 23, 2018
@fpagnoux fpagnoux changed the title Don't force persons to be assigned to all defined entitiesd Don't force persons to be assigned to all defined entities Aug 23, 2018
@fpagnoux
Copy link
Member

The limit of the current model is also problematic for Barcelona:

from @jvalduvieco:

I have a benefit that includes parents and sons. And In some families were grandparents, sons and grandsons live in the same household I has planning to have two families one for parents and childs and another for childs and grandchilds. (from the point of view of the grandparent.

image

@bonjourmauko bonjourmauko added the kind:feat A feature request, a feature deprecation label Oct 5, 2018
@fpagnoux
Copy link
Member

fpagnoux commented Nov 5, 2018

More input from @jvalduvieco:

  • All group entities defined in all use cases - I understand this is a requirement due internal core implementation but ties all the tests to all group entities defined in the system. This makes tests fragile and is one of my pain points (for example in our system we have 2nd_grade_families, parents_and_children_families, persons_living_together group entities. persons_living_together only applies to a certain set of formulas but I need to define this for all formulas).
  • All persons in all entities - This forces me to create other* roles to include non relevant persons for a group entity (for example I need to add persons who are not part of parents_and_children_families with a role like other_persons to satisfy this requirement).

@bonjourmauko
Copy link
Member

I'm reopening this issue as there seems to be two different problems, 1. requiring people to be assigned to a group entity, and 2. allowing people to be assigned to several group entities, of whom only the former is solved.

If you think we should rather split the issue, I'll happily do so.

@bonjourmauko bonjourmauko reopened this Apr 23, 2021
@MattiSG
Copy link
Member

MattiSG commented Jul 27, 2021

If you think we should rather split the issue, I'll happily do so.

I'm indeed not so sure I understand why this issue, titled “Don't force persons to be assigned to all defined entities”, was not solved by #835 🤔 I believe that the issue reported by the OP could not happen anymore and has been solved.

The discussion here might well give more context to the second feature you describe, but I believe it would be better to create its own issue with a specific, real-life example 🙂 Is it okay to close this issue now?

@MattiSG
Copy link
Member

MattiSG commented Aug 9, 2021

Assuming it is okay to close the issue as explained above, cc @maukoquiroga.

@MattiSG MattiSG closed this as completed Aug 9, 2021
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

No branches or pull requests

7 participants