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

Input variables with an ETERNITY period require a month to be specified #669

Closed
jmhbnz opened this issue May 19, 2018 · 8 comments
Closed
Assignees

Comments

@jmhbnz
Copy link

jmhbnz commented May 19, 2018

Hi there!

When setting an ETERNITY input variable I was not expecting to have to define a month.

Here is my input variable:

class birth(Variable):
    value_type = date
    default_value = date(1970, 1, 1)  # By default, is no value is set for a simulation, we consider the people involved in a simulation to be born on the 1st of Jan 1970.
    entity = Person
    label = u"Birth date"
    definition_period = ETERNITY  # This variable cannot change over time.
    reference = u"https://en.wiktionary.org/wiki/birthdate"

Here is what is required to set that input via API:

"birth" : {
	        	"2018-05" : 
	        	"2012-04-01"},

For an ETERNITY value I believe specifying a month shouldn't be necessary?

@jmhbnz jmhbnz changed the title Input variablrs with an ETERNITY period require a month to be specified Input variables with an ETERNITY period require a month to be specified May 19, 2018
@MattiSG
Copy link
Member

MattiSG commented May 19, 2018

Thanks @jmhbnz for having taken the time to document this issue! 😃

I notice that France's Mes Aides also defines ETERNITY variables like date_naissance (date of birth) by defining the value for each month, which sounds absurd: https://mes-aides.gouv.fr/api/situations/56a8f80697f543fe233ee40c/openfisca-request

I would expect this to be declared just as "birth" : "2012-04-01", with no nested object.

@fpagnoux
Copy link
Member

Agreed, but I think the point that was raised during the Web API design was to have homogenous objects.

Allowing the suggested shortcut would give up inhomogenous inputs (and possibly outputs) such as:

"birth": "1987-04-01",
"salary": {
    "2017-01": 2000
}

I think the main way of dealing with ETERNITY defined variable was imagined to be more like that:

"birth": {
    "ETERNITY": "1987-04-01"
},
"salary": {
    "2017-01": 2000
}

which is less "absurd", more semantic but... heavy and not really intuitive.

In practice, users such as mes-aides just put a random month instead of explicitly using the "ETERNITY" key, because it gives the same result, and is probably less work on their side.

@MattiSG
Copy link
Member

MattiSG commented May 23, 2018

I think the main way of dealing with ETERNITY defined variable was imagined to be more like that:

Haaahahaa yes it was. Actually, that's totally the aim: you can only use a date or ETERNITY as key, but the error message only mentions dates.
I guess we just failed at documentation 😓

So I suggest we just go with improving documentation and error message and see if that re-surfaces later.

@MattiSG
Copy link
Member

MattiSG commented May 23, 2018

Actually I find this to be quite related to openfisca/openfisca-doc#141 (and loosely to #670 as well considering just how many practices were mentioned there that I did not know about). We need a pass on the periods documentation.

@fpagnoux
Copy link
Member

@MattiSG do you thing adding an example with ETERNITY in the error message would be enough to make it clear? Something like:

{
  "persons": {
    "Alicia": {
      "birth": "Invalid type: must be of type object. Input variables must be set for specific periods. For instance: {'salary': {'2017-01': 2000, '2017-02': 2500}}, or {'birth_date': {'ETERNITY': '1980-01-01'}}."
    }
  }
}

@fpagnoux
Copy link
Member

PR opened: #688.
Comments and suggestions welcome 🙂

@MattiSG
Copy link
Member

MattiSG commented Jul 11, 2018

do you thing adding an example with ETERNITY in the error message

With documentation in the API section, that would be enough for me, yes:

I suggest we just go with improving documentation and error message and see if that re-surfaces later.
#669 (comment)

@fpagnoux
Copy link
Member

fpagnoux commented Jul 12, 2018

Ok. Feel free to add any suggestion on how you think the periods doc should evolve on openfisca/openfisca-doc#141.

I'm meanwhile closing this issue, as I think the question has been answered, and the improvement suggestions have been documented in other issues or merged. Feel free to re-open it if you think it's not the case 🙂 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants