Switch parameters to YAML #552

michelbl · 2017-08-03T08:26:02Z

Connected to #523

See changelog (TODO: copy the changelog here after merge)

michelbl · 2017-08-03T13:50:15Z

Still to do :

Update scripts to import IPP parameters. There are already a script to convert XLSX files from IPP to XML, a script to convert XML to YAML and a script to merge XML. What is needed is to combine the former 2 and update the later to YAML. I think this has not to be done in this PR.
Write a script to find expected values. I think this has not to be done in this PR.
The french reform Landais-Piketty-Saez uses a LinearAverageTaxScale. This scale cannot be declared with the current YAML syntax. What should we do (some solutions:)
- Add a keyword to YAML files
- Remove the Landais-Piketty-Saez from Openfisca-France
- Find a way to express this scale using the interface
- Allow to create this type of scale but not from YAML (using the constructor of class Scale). This is the closest solution to what exists now.
A test broke with no link to parameters. I need to investigate with the clue that @fpagnoux gave me.
Rename legislation to parameters in internal variables. This has to be debated and this has not to be done in this PR.
OpenFisca-WebAPI requires OpenFisca-Core < 0.17. https://github.com/openfisca/openfisca-web-api/blob/master/setup.py#L63

Anna-Livia · 2017-08-03T15:13:54Z

openfisca_core/reforms.py

-        Copy the baseline TaxBenefitSystem legislation_json attribute and return it.
-        Used by reforms which need to modify the legislation_json, usually in the build_reform() function.
-        Validates the new legislation.
+        Used by reforms which need to modify the legislation parameters.


Use if the reform asks for legislation parameter modification

Anna-Livia · 2017-08-03T15:18:05Z

openfisca_core/reforms.py

 class Reform(TaxBenefitSystem):
+    """A modified TaxBenefitSystem
+
+    A reformed TaxBenefitSystem must subclass `Reform` and implement a method `apply()`. Such a function can add or replace variables and call `self.modify_legislation()` to modify the parameters of the legislation.


In OpenFisca, a reform is a modified TaxBenefitSystem.
It can add or replace variables and call self.modify_legislation()to modify the parameters of the legislation. All reforms must subclassReformand implement aapply()` method .

Anna-Livia · 2017-08-03T15:25:32Z

openfisca_core/taxbenefitsystems.py

@@ -48,10 +55,10 @@ class VariableNameConflict(Exception):

 class TaxBenefitSystem(object):
    """
-    Represents the legislation.
+    Represents the legislation. It stores parameters (values defined for everyone) and variables (values defined for some given entity e.g. a person).


J'enlèverait "Represents the legislation" --> cela reste déroutant, quand legislation peut représenter seulement les paramètres dans certain contextes

C'est plutôt dans l'autre sens qu'il faudrait aller: le TBS représente la législation, mais les paramètres sont... des paramètres.

ok, je ne change rien

fpagnoux

I did not check the code deeply, but here is my first review.

The main thing that puzzles me is the type attribute. This was not mentioned in the issue discussions, and I find it a little heavy (espacially the almost empty files that need to be put in each intermediate directory)

fpagnoux · 2017-08-04T12:07:28Z

CHANGELOG.md

+
+  Now:
+  ```yaml
+  type: node


Is that necessary ?

Can't we figure out if it's a nose just by checking its properties ?

fpagnoux · 2017-08-04T12:08:22Z

CHANGELOG.md

+      descrption: Taux d'impôt sur les salaires
+      unit: /1
+      values:
+        '2016-01-01':


Are the quotes necessary ?

fpagnoux · 2017-08-04T12:08:26Z

CHANGELOG.md

+    type: node
+    taux:
+      type: parameter
+      descrption: Taux d'impôt sur les salaires


fpagnoux · 2017-08-04T12:09:02Z

CHANGELOG.md

+  impot:
+    type: node
+    taux:
+      type: parameter


Is that necessary ?

Can't we figure out if it's a nose just by checking its properties ?

fpagnoux · 2017-08-04T12:14:32Z

CHANGELOG.md

+
+  - The XML attributes `format` and `type` are replaced by the YAML attribute `unit` than can take as values `year`, `currency` and `/1`.
+
+* Refactor the internal representation and the interface of legislation parameters


In the "Breaking changes" part, as a user, I want to check if something that I was using has been removed/changed.

Reading this part doesn't help me much. I know something has been refactored (by that's not really my business), and I know that new classes have been introduced. But what former classes/methods/functions/attributes have been removed ?

moved to another section

fpagnoux · 2017-08-04T12:56:33Z

CHANGELOG.md

+#### Technical changes
+
+* The variables `*legislation_json*` are replaced by `*legislation*` because the format json is no longer used.
+* The expression "compact legislation" is replaced by the expression "legislation at instant". For example, `taxbenefitsystem.compact_legislation_by_instant_cache` is renamed as `taxbenefitsystem.legislation_at_instant_cache`.


Isn't that a breaking change ?

I clarified it a bit

fpagnoux · 2017-08-04T12:59:03Z

openfisca_core/simulations.py

    debug = False
    debug_all = False  # When False, log only formula calls with non-default parameters.
    period = None
-    baseline_compact_legislation_by_instant_cache = None
+    baseline_legislation_at_instant_cache = None


This breaking change must be documented in the changelog.
Maybe we should also make this attribute private ?

fpagnoux · 2017-08-04T13:00:27Z

openfisca_core/simulations.py

@@ -11,11 +11,11 @@


 class Simulation(object):
-    compact_legislation_by_instant_cache = None
+    legislation_at_instant_cache = None


This breaking change must be documented in the changelog.

Maybe we should also make this attribute private ?

fpagnoux · 2017-08-04T13:03:07Z

openfisca_core/taxbenefitsystems.py

@@ -48,10 +55,10 @@ class VariableNameConflict(Exception):

 class TaxBenefitSystem(object):
    """
-    Represents the legislation.
+    Represents the legislation. It stores parameters (values defined for everyone) and variables (values defined for some given entity e.g. a person).


C'est plutôt dans l'autre sens qu'il faudrait aller: le TBS représente la législation, mais les paramètres sont... des paramètres.

fpagnoux · 2017-08-04T13:04:57Z

openfisca_core/taxbenefitsystems.py

        # TODO: Currently: Don't use a weakref, because they are cleared by Paste (at least) at each call.
-        self.compact_legislation_by_instant_cache = {}  # weakref.WeakValueDictionary()
+        self.legislation_at_instant_cache = {}  # weakref.WeakValueDictionary()


As we are renaming it, should we be opportunistic and rename it to parameters_at_instant_cache ?

MattiSG · 2017-08-16T12:01:23Z

Replace mandatory _.yaml by optional index.yaml.
Remove the need to specify the type property.

MattiSG · 2017-08-16T14:08:59Z

Update scripts to import IPP parameters.

Ok to postpone, but that is quite important to get back.

Write a script to find expected values.

Ok to postpone.

The french reform Landais-Piketty-Saez uses a LinearAverageTaxScale. This scale cannot be declared with the current YAML syntax.

This is used only in one reform at the moment, but the economical logic behind it is not unique. In this case, replace the rate property by average_rate and load accordingly.

A test broke with no link to parameters.
OpenFisca-WebAPI requires OpenFisca-Core < 0.17.

It seems to be the Web API, which cannot serve the type and format properties anymore.

If regaining the type property is easy thanks to Python introspection, let's bring it back and not fill in the format optional property at all (or expose the unit contents).

We just need to obtain the list of currently exposed type parameters. This can be done by inspecting the swagger endpoint.

Rename legislation_json to parameters in internal variables.

@benjello is this ok with you? It does seem more readable to me to have parameters rather than legislation, would it be only for similarity with variables, and avoiding the ambiguity between legislation and tax_benefit_system.

benjello · 2017-08-17T14:57:30Z

@MattiSG : the trade-off for me is choosing the less puzzling ambiguity:

legislation/tax_benefit_system
variables/parameters

I do not have a clear preference ;-) so it is ok change legislation in parameters

michelbl · 2017-08-18T20:01:56Z

Thanks for your reviews.

I'm not sure about how the caching of the parameters should be tested.

MattiSG · 2017-08-21T12:41:42Z

For caching:

The pickle cache must not be versioned. It can be packaged and deployed, but not be versioned in the source.
If we choose not to systematically validate, we'll validate on the first exception in the constructors and output either the validation result or a recommendation to open an issue (as an exception with no validation error means there is most probably an issue in the core itself).
A separate command for validation can be provided, or an option in all CLI commands.

The current options seem to be:

Caching: 50 ms.
Parsing with libyaml: 500 ms.
Parsing with pyyaml: 3 000 ms.
Parsing & validation with libyaml: 8 500 ms.
Parsing & validation with pyyaml: 12 000 ms.

In which case, I favour optional validation and recommendation to install libyaml over caching, as caching means we have to design deployment and, worst of all, cache invalidation.

fpagnoux · 2017-08-28T16:12:46Z

CHANGELOG.md

+          value: 0.3
+  ```
+
+  - The XML attributes `format` and `type` are replaced by the YAML attribute `unit` than can take as values `year`, `currency` and `/1`.


Let's keep it a free text field for now.

fpagnoux · 2017-08-28T16:18:22Z

CHANGELOG.md

+    ```
+  - Now:
+    ```python
+    parameters.impot_revenu.bareme[1].threshold.update(period=reform_period, value=6011)


Spaces around =

fpagnoux · 2017-08-28T16:21:12Z

CHANGELOG.md

+    from openfisca_core.parameters import ParameterNode
+
+    inflation = .001
+    reform_parameters_subtree = Node('plf2016_conterfactual', validated_yaml = {


ParameterNode

fpagnoux · 2017-08-28T16:21:38Z

CHANGELOG.md

+
+  - This way of creating parameters is deprecated (see below), except when using dynamically computed values. It is the case in the example because `round(1135 * (1 + inflation))` is computed at run time.
+
+* The function `reforms.compose_reforms()` is removed.


remove function reforms.compose_reforms()

fpagnoux · 2017-08-28T16:32:04Z

openfisca_core/parameters.py

+        self.values_list = values_list
+
+    def __repr__(self):
+        return 'ValuesHistory "{}"\n'.format(self.name) + ''.join('  {}: {}\n'.format(value.instant_str, value.value) for value in self.values_list)


Use os.linesep

fpagnoux · 2017-08-28T16:41:20Z

openfisca_core/reforms.py

-        i += 1
-
-    return new_items
+        assert isinstance(reform_parameters, ParameterNode)


No. If this is false, there will be no exploitable error message. Don't use assertions.

fpagnoux · 2017-08-28T16:45:49Z

openfisca_core/simulations.py

+            return self._get_baseline_parameters_at_instant(instant)
+        return self._get_parameters_at_instant(instant)
+
+    def legislation_at(self, instant, use_baseline = False):


Not sure this retro-compatibility layer is really necessary

fpagnoux · 2017-08-28T16:50:32Z

openfisca_core/taxbenefitsystems.py

+
+        self.parameters = parameters
+
+    def get_parameters(self):


I think parameters is clean enough so that we can expose it directly

fpagnoux · 2017-08-28T16:50:57Z

openfisca_core/taxbenefitsystems.py

+        """
+        return self.parameters
+
+    def get_legislation(self):


Is this retro-compatibility layer really necessary ?

fpagnoux · 2017-08-28T16:53:14Z

openfisca_web_api_preview/loader/parameters.py

-                path_fragments = path_fragments + [child_name],
-                )
+    for child_name, child in children.items():
+        child_type = type(child).__name__


Import the types instead of using the names

michelbl changed the title ~~Params yaml~~ Switch parameters to YAML Aug 3, 2017

michelbl force-pushed the params-yaml branch from 18870ec to 0d7624e Compare August 3, 2017 13:20

michelbl requested review from fpagnoux and Anna-Livia August 3, 2017 15:14

michelbl self-assigned this Aug 3, 2017

Anna-Livia reviewed Aug 3, 2017

View reviewed changes

fpagnoux requested changes Aug 4, 2017

View reviewed changes

MattiSG mentioned this pull request Aug 17, 2017

Adapt to OpenFisca-Core 17.0.0 openfisca/openfisca-web-api#142

Merged

michelbl force-pushed the params-yaml branch from c610081 to 85be82d Compare August 18, 2017 20:43

michelbl requested review from MattiSG, fpagnoux and Anna-Livia August 18, 2017 20:45

fpagnoux requested changes Aug 28, 2017

View reviewed changes

fpagnoux force-pushed the params-yaml branch 2 times, most recently from ea25cca to fd63109 Compare August 28, 2017 17:47

fpagnoux approved these changes Aug 28, 2017

View reviewed changes

fpagnoux removed request for MattiSG and Anna-Livia August 28, 2017 18:19

fpagnoux force-pushed the params-yaml branch from ccd0403 to 86334d5 Compare August 28, 2017 19:11

Michel Blancard added 5 commits August 28, 2017 22:09

Move XML parser to scripts dir

002ab4c

Fix dependency to XSD file

60ae1d7

Output parameters as YAML files

50bbefc

Parse YAML parameters

8e6f06c

Load parameters from YAML instead of XML

be70688

fpagnoux added 26 commits August 28, 2017 23:01

Refactor tests

0eb3840

Test parameter validation

c736893

Refactor validation

06518a0

Make '' the default name of a ParameterNode

236f7de

Fix extensions param loading

bdf2bab

Fix ValueHistory.update

3e777e6

Remove web API doc

07d8782

Improve ref doc

9d5d678

Remove assertions

3a827ad

Expose tab_benefit_system.parameters

cf93d38

Enable parameter.update

08fdacd

Improve Changelog

556693d

Make ValueHistory repr work cross platform

fdff60d

Improve reforms ref doc

7c594f0

Don't accept index.yaml

d8ef5f5

Don't use assertions

9c14510

Remove retro-compatibility layers

2f6e662

Don't rely on type names

74a6453

Rename yaml_object to data

facc774

Enable auto-completion in node exploring

19b2dea

Allow description for nodes and index.yaml files

8a19ba4

Make tbs.get_parameters_at_instant public

39ba0b4

Apply renaming

c7cf5f7

Fix rebase mistakes

f9fce0f

Fix migration scripts

b3456c0

Upgrade country package version

4a74f80

fpagnoux force-pushed the params-yaml branch from 403d1b5 to 4a74f80 Compare August 28, 2017 21:02

fpagnoux merged commit 4e50320 into master Aug 28, 2017

fpagnoux deleted the params-yaml branch August 28, 2017 21:06

MattiSG mentioned this pull request Dec 10, 2021

Support str and/or date parameters #1092

Open


		- The XML attributes `format` and `type` are replaced by the YAML attribute `unit` than can take as values `year`, `currency` and `/1`.

		* Refactor the internal representation and the interface of legislation parameters


		- This way of creating parameters is deprecated (see below), except when using dynamically computed values. It is the case in the example because `round(1135 * (1 + inflation))` is computed at run time.

		* The function `reforms.compose_reforms()` is removed.

Switch parameters to YAML #552

Switch parameters to YAML #552

Conversation

michelbl commented Aug 3, 2017 • edited Loading

michelbl commented Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michelbl Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattiSG commented Aug 16, 2017

MattiSG commented Aug 16, 2017

benjello commented Aug 17, 2017

michelbl commented Aug 18, 2017

MattiSG commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michelbl commented Aug 3, 2017 •

edited

Loading

michelbl commented Aug 3, 2017 •

edited

Loading

michelbl Aug 18, 2017 •

edited

Loading