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

Merge Variable and DatedVariable #371

Closed
cbenz opened this issue Jan 18, 2016 · 10 comments · Fixed by #522
Closed

Merge Variable and DatedVariable #371

cbenz opened this issue Jan 18, 2016 · 10 comments · Fixed by #522

Comments

@cbenz
Copy link
Member

cbenz commented Jan 18, 2016

See openfisca/openfisca-france#376 (comment)

When the decorator dated_function is used on a Variable like this:

class cehr(Variable):
    [...]

    @dated_function(start = date(2011, 1, 1), stop = date(2011, 12, 31))
    def function(self, simulation, period):
        return period, [...]

    @dated_function(start = date(2012, 1, 1))
    def function(self, simulation, period):
        return period, [...]

the Core should detect that it's a "dated variable" automatically.

@cbenz
Copy link
Member Author

cbenz commented Jan 18, 2016

Or have only Variable classes which behave differently whether @dated_function is used or not.

@fpagnoux
Copy link
Member

I like the second option, it makes the API simpler :)

@cbenz cbenz changed the title Fail when using dated_function without DatedVariable Merge Varable and DatedVariable Jan 22, 2016
@cbenz
Copy link
Member Author

cbenz commented Jan 22, 2016

@fpagnoux I changed the issue title and description

@benjello benjello changed the title Merge Varable and DatedVariable Merge Variable and DatedVariable Feb 1, 2017
@fpagnoux
Copy link
Member

fpagnoux commented Apr 26, 2017

Following a discussion with @MattiSG and personal thoughts:

  • Variable and DatedVariable should be merged (both the API and the internal structure).
    • We just have some Variable with one formula, and others with several ones.
  • The start_date and stop_date variable attributes should be deprecated, as they are redundant with the decorators.
    • Some input variables have a start_date and a stop_date. This has no impact on the behaviour, as the only effect of this attribute is to deactivate the formulas. We should investigate if they have a real meaning.
  • The decorator should be simplified to something like @start(2012), @start(2012, 12) or @start('2012-02') (to discuss).
  • We should incentive formula writers to use the similar stop decorator only to signify that a variable has no formula after a given date. It should not be added if it is redundant with the start value of the next formula. The modality of the incentive is to be determined (doc, warning, error...).
  • It should be possible to give the same name to two formulas within a variables. Currently, the first dated formula would be ignored, which is a mean trap for formula writers.

@benjello
Copy link
Member

benjello commented Apr 26, 2017

The variable that have no formula but a start/stop date is typically a box of the income tax form that doesn't last forever. You should not be able to set it even if it doesn't affect the computation.
And knowing its existence period is information you want to keep.

@MattiSG
Copy link
Member

MattiSG commented Apr 28, 2017

It should not be added if it is redundant with the start value of the next formula.

Solution: support stop only at the Variable level, not formula.

@MattiSG
Copy link
Member

MattiSG commented May 2, 2017

@fpagnoux
Copy link
Member

fpagnoux commented May 11, 2017

Solution: support stop only at the Variable level, not formula.

I wonder if in some cases, we want to stop the computation of a variable through its formulas without deactivating the variable itself.

sandcha added a commit that referenced this issue May 11, 2017
@MattiSG
Copy link
Member

MattiSG commented May 16, 2017

in some cases

Any example? If I recall correctly, it seems that a “temporary” removal of a variable from law which would be brought back later on, which seems like the main use case, is usually already implemented by creating a new variable to avoid confusion.

@MattiSG
Copy link
Member

MattiSG commented May 18, 2017

  • end = 'YYYY-MM-DD' attribute on a variable if it is removed from the tax and benefit system (similarity with the <END> node).
  • Rename function and @start_date annotation to formula[_YYYY[_MM[_DD]]] (default YYYY to 0001, MM and DD to 01).
  • Comment out all start_date attributes on variables with no formula. This had no impact already, but contains valuable metadata.
  • Do not assert that a DatedVariable can not be defined on ETERNITY.

sandcha added a commit that referenced this issue May 30, 2017
Split all tests into test_call/test_dates.
Vectorize dummy functions output.
Update tests so that they all pass > Represents variable dates
management contract before #371 changes.
fpagnoux pushed a commit that referenced this issue May 31, 2017
fpagnoux pushed a commit that referenced this issue May 31, 2017
Split all tests into test_call/test_dates.
Vectorize dummy functions output.
Update tests so that they all pass > Represents variable dates
management contract before #371 changes.
fpagnoux pushed a commit that referenced this issue Jun 12, 2017
fpagnoux pushed a commit that referenced this issue Jun 12, 2017
Split all tests into test_call/test_dates.
Vectorize dummy functions output.
Update tests so that they all pass > Represents variable dates
management contract before #371 changes.
@fpagnoux fpagnoux reopened this Jun 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants