Re-add support for dispatch/divide by days

openfisca / openfisca-core

OpenFisca core engine. See other repositories for countries-specific code & data.

https://openfisca.org

GNU Affero General Public License v3.0

168 stars 75 forks source link

Re-add support for dispatch/divide by days #1143

Closed bonjourmauko closed 1 year ago

bonjourmauko commented 2 years ago

Fixes govzeroaotearoa/openfisca-aotearoa#15 Depends on #1141

New features

Introduce support for the day date unit in holders.set_input_dispatch_by_period and holders.set_input_divide_by_period
- Allows for dispatching values per day, for example, to provide a daily (week, fortnight) to an yearly variable.
- Inversely, allows for calculating the daily (week, fortnight) value of a yearly input.

MattiSG commented 2 years ago

I will postpone reviewing this PR until the one it depends on is approved, and suggest that this one is set to draft. Indeed, I fail to see how it could be approved when #1141 might not go through.

Do you truly need #1141 for this one to be reviewed? 🙂 As an alternative to switching to draft, I suggest to remove the dependency and rebase straight on master. Yes, test-docs might fail until #1141 is fixed, but that is true of all PRs, not only of this one. This would enable reviews to be parallelised and proper solutions found for #1140 without blocking all the others.

bonjourmauko commented 2 years ago

Done!

bonjourmauko commented 2 years ago

I'm not sure if this is a new feature, or a bug-fix as the behaviour this adds was introduced as a replacement of a behaviour that was deprecated, however the pull request forgot to include days in that migration.

benjello commented 2 years ago

@maukoquiroga : looks very good to me. Just a question: how does this PR handles month day size, february and leeap year for divide_by_period ? It does not seem to be tested although there are a bunch of tests for that in Period and Instant.

bonjourmauko commented 2 years ago

@maukoquiroga : looks very good to me. Just a question: how does this PR handles month day size, february and leeap year for divide_by_period ? It does not seem to be tested although there are a bunch of tests for that in Period and Instant.

I didn't do a lot more than «reenabling» a feature that already existed.

If you see in the tests, month days are handled appropriately (31 + 28 + 30).

Maybe test should be more precise about leap years.

bonjourmauko commented 2 years ago

(By the way, I think that holders implements it's own logic in terms of equivalences between days, months and years. We should see which is better, and keep just one in periods, as holder should not handle that kind of logic IMHO).

bonjourmauko commented 1 year ago

@benjello @MattiSG could you please take a look at this pull request again?

bonjourmauko commented 1 year ago

cc @MattiSG @benjello