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

Uniformize variable declaration : allow inputs to be declared as a formula without a function #348

Closed
wants to merge 1 commit into from

Conversation

fpagnoux
Copy link
Member

As discussed with @eraviart last Friday, declaring formulas and input in different ways has a few drawbacks :

  • More complexity for the developer, who has to know 2 ways of declaring a variable that look like each other with a slightly different syntax.
  • Possible mistakes (comas are needed in one syntax, not the other)
  • When looking for a variable declaration with a global text research, inputs are hard to find.

It would be nice if we could have only one syntax. A possible implementation (started here) is to just allow SimpleFormulaColumn to not have a function.

Before :

reference_input_variable(
    name = "ass_precondition_remplie",
    column = BoolCol,
    entity_class = Individus,
    label = u"Eligible à l'ASS",
    )

@reference_formula
class ass(SimpleFormulaColumn):
    column = FloatCol
    label = u"Montant de l'ASS pour une famille"
    entity_class = Familles

    def function(self, simulation, period):
         return period, x

After :

@reference_formula
class ass_precondition_remplie(SimpleFormulaColumn):
    column = BoolCol
    entity_class = Individus
    label = u"Eligible à l'ASS"

@reference_formula
class ass(SimpleFormulaColumn):
    column = FloatCol
    label = u"Montant de l'ASS pour une famille"
    entity_class = Familles

    def function(self, simulation, period):
         return period, x

@fpagnoux
Copy link
Member Author

I'm not sure entirely sure this is ready to merge right now, but tests seem to be ok on my computer. Maybe the core team to have a look to make sure everything is not broken :)

@benjello
Copy link
Member

Why not but one should rename formula to variable somehow. A formula without a function is a bit awkward.

@fpagnoux
Copy link
Member Author

Good point, thanks !

Something like that ?

@reference_variable
class ass_precondition_remplie(SimpleVariable):
    column = BoolCol
    entity_class = Individus
    label = u"Eligible à l'ASS"

Btw, what does reference mean in reference_formula ?

@benjello
Copy link
Member

reference is for "root" tax_benefit_system vs reform IIRC @cbenz @eraviart

@fpagnoux
Copy link
Member Author

Ok, I understand better, thanks :)

@cbenz
Copy link
Member

cbenz commented Sep 29, 2015

Furthermore I would suggest renaming even @reference_formula to @reference_variable, because a variable can have or not a formula. The formula is the function.

We could even replace the term reference by either vanilla (like @benjello says often), country or tax_benefit_system

@fpagnoux
Copy link
Member Author

fpagnoux commented Oct 9, 2015

@vanilla_variable does not seem very explicit to me. What do you think about @law_variable ?

@benjello
Copy link
Member

benjello commented Oct 9, 2015

@reference_variable is good for me

@fpagnoux
Copy link
Member Author

fpagnoux commented Oct 9, 2015

I tend to agree with @cbenz that for contributors who are only using Openfisca to run simulation about the actual law and are not trying potential reforms (e.g. embauche or mes-aides), the term reference doesn't mean much and is pretty unclear.

Ideally, it would be nice to have a term that makes sense both for the distinction reference vs reform, and for use cases without reform.

@benjello
Copy link
Member

benjello commented Oct 9, 2015

If we use core_variable it will mess-up with openfisca-core.
Should we use reference_variable or base_variable and have two distinct term for reform and extension ?

@cbenz
Copy link
Member

cbenz commented Oct 23, 2015

I close this pull request but opened a new issue: #355

@cbenz cbenz closed this Oct 23, 2015
@fpagnoux fpagnoux deleted the uniformize-variable-declaration branch October 23, 2015 16:07
@fpagnoux fpagnoux restored the uniformize-variable-declaration branch December 3, 2015 14:54
@cbenz cbenz self-assigned this Dec 3, 2015
@cbenz cbenz added this to the v1.0 milestone Dec 3, 2015
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

Successfully merging this pull request may close these issues.

3 participants