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

Added example script for running the situation examples #149

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

verbman
Copy link
Contributor

@verbman verbman commented May 13, 2024

  • Technical improvement
  • Details:
    • Add run.py to situation examples, enables simple executable situation example for documentation.

The OpenFisca docs link to a France specific example here that is out of date and should use SimulationBuilder. This PR adapts the situation examples example from the tutorial and applies it here. This would provide us the opportunity to provide simple code tests to the documentation that isn't France specific. For instance here we could instead use this repository as the example and provide the script python -m openfisca_country_template.situation_examples.run to prove installation.

@verbman verbman requested a review from MattiSG May 13, 2024 03:18
@MattiSG
Copy link
Member

MattiSG commented May 17, 2024

Thank you for this suggestion.

The doc page you linked to is called “How to test your changes on “ready to use” situations (for OpenFisca-France)”. It is clearly country-specific and should be erased as such.
The idea it points at, however, is relevant and interesting: the idea of ensuring each country model shares and maintains these “ready to use situations”, or “cas types” in French (apparently translated as “typical cases”) for reuse.
It is already in large part implemented by the existence of these “situation examples”.
I would suggest renaming this folder to typical_cases to surface their usage, and adapt the documentation to point that every country package is encouraged to provide their own typical cases, and that their default location is in this folder.

I am not to see the point of providing trivial Python code to then load these typical cases. If you believe it can be worthwhile, I would suggest making this file an actual part of the test suite to systematically validate the validity of the typical cases 🙂 In this way, every country model would be pushed to maintain them. Ideally, we would find a way to check this validity without requesting calculations that depend on the model variables (i.e. just make sure that every input variable referenced in the typical case is a valid one, without requesting any variable to be computed, so that the tests don't need to be updated when the output variables would change).

Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure that all automated checks pass before requesting a review, in order to avoid unnecessary repeated reviews and ensure that the reviewed codebase is a functional version that abides by quality standards 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this file is ambiguous and unclear. As I understand it, it is only an executable example of how to load situation examples. However its name, run, makes it sound as though it is the one way to access or build the examples.

@@ -0,0 +1,12 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the doc, “Python supports writing source code in UTF-8 by default”, are you sure this is necessary?

@bonjourmauko bonjourmauko added the kind:feat A new feature or solution label Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feat A new feature or solution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants