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

チュートリアルの作成 #69

Merged
merged 9 commits into from
Dec 27, 2023

Conversation

AZKZ
Copy link
Contributor

@AZKZ AZKZ commented Aug 6, 2023

Pull request前の確認

  • フォークしたリポジトリのdevelopブランチ(あるいはそこから新たに切ったブランチ)にプッシュを行った。(mainブランチに直接プッシュしていない。)
  • プルリクエストでマージを要求するブランチをmainブランチからdevelopブランチに変更した。

概要

  • この Pull request は チュートリアルを作成しました
    • 今後、使うことになりそうなので 小学生学年(Enum) を作成しました

動作確認

@netlify
Copy link

netlify bot commented Aug 6, 2023

Deploy Preview for shien-yadokari canceled.

Name Link
🔨 Latest commit a3cdd81
🔍 Latest deploy log https://app.netlify.com/sites/shien-yadokari/deploys/658b6e1a258e5700083fcb87

docs/tutorial.md Outdated

テストデータを作成するには下記の作業を実施します。

1. テストパターンのCSVを作成する
Copy link
Contributor

Choose a reason for hiding this comment

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

「3の倍数給付金」おもしろいですね!一連の流れがわかると着手しやすくなりますね。

児童手当等はcsv -> yamlのテストファイル変換をしていたのですが、最近対象とした制度はロジックが複雑すぎるため、細かいステップごとの単体テストを全てyamlで直接作っています。(例:openfisca_japan/tests/福祉/生活保護)
そのため、CSVをyamlに変換する手順はなくても問題ありません。

@AZKZ AZKZ force-pushed the #48_create_tutorial branch from 6442f40 to 4c1f5ef Compare November 28, 2023 11:29
Copy link

netlify bot commented Nov 28, 2023

Deploy Preview for silly-gumption-65c982 canceled.

Name Link
🔨 Latest commit 16f19c8
🔍 Latest deploy log https://app.netlify.com/sites/silly-gumption-65c982/deploys/6572694cef78000008774c3d

@AZKZ AZKZ force-pushed the #48_create_tutorial branch 3 times, most recently from 0143bcb to 20f2a27 Compare December 7, 2023 15:35
@AZKZ AZKZ force-pushed the #48_create_tutorial branch from 20f2a27 to 37919d6 Compare December 7, 2023 15:41
@AZKZ AZKZ changed the title 【編集中】チュートリアルの作成 チュートリアルの作成 Dec 7, 2023
@AZKZ AZKZ marked this pull request as ready for review December 7, 2023 15:46
@AZKZ
Copy link
Contributor Author

AZKZ commented Dec 7, 2023

@SnoozingJellyfish

チュートリアルを作成したので、お手数ですがご確認をお願い致します!

Copy link
Contributor

@SnoozingJellyfish SnoozingJellyfish left a comment

Choose a reason for hiding this comment

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

丁寧なチュートリアルありがとうございます!わかりやすく面白いですね。
いくつかコメントしています。 markdownでプルダウンができるのを初めて知りました。

@syupern さん、補足やより良い説明方法、修正した方が良い点等あればコメントお願いします!

docs/tutorial.md Outdated
class 三の倍数給付金(Variable):
value_type = int # この給付金は小数にはならないので整数型のintを指定している
entity = 世帯 # この給付金は世帯ごとに支給されるので世帯を指定している
definition_period = DAY # 基本はDAYを指定する。※どういう時にYEARやMONTHを指定する?
Copy link
Contributor

Choose a reason for hiding this comment

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

YEARやMONTHの使い方はよくわかってないです。。

Copy link
Contributor

@Syuparn Syuparn Dec 9, 2023

Choose a reason for hiding this comment

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

Variable間で期間を換算する必要が無いためDAYに揃えているのではないかと思います。

OpenFisca公式のドキュメントによると制度が定義されている期間を設定するものなので、それに倣うのが良さそうです。(年間10万円なので YEAR ?)
#184

docs/tutorial.md Outdated
年齢が3の倍数の世帯員数 = np.count_nonzero(np.mod(世帯員の年齢一覧, 3) == 0)

# 3の倍数の世帯員の数×100,000円を返す
給付金額 = 年齢が3の倍数の世帯員数 * 100000
Copy link
Contributor

Choose a reason for hiding this comment

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

定数(100000円)をparameters/フォルダにyamlで定義する手順も記載お願いします!

docs/tutorial.md Outdated
世帯員の年齢一覧 = 対象世帯.members("年齢", 対象期間)

# 3の倍数の世帯員の数を取得する
年齢が3の倍数の世帯員数 = np.count_nonzero(np.mod(世帯員の年齢一覧, 3) == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

OpenFiscaの推奨する書き方としては以下の感じです。

年齢が3の倍数である = 世帯員の年齢一覧 % 3 == 0   # 条件に該当するか否かのbool値のndarray配列ができる
給付金額一覧 = 年齢が3の倍数である * (定数)   #  世帯員ごとに給付される額が格納されたndarray配列ができる
給付金額 = 対象世帯.sum(給付金額一覧)   # numpy の関数はできるだけ使わないことが望ましい

docs/tutorial.md Outdated
世帯員の学年一覧 = 対象世帯.members("学年", 対象期間)

# ボーナス対象学年の世帯員数を取得する
ボーナス対象学年の世帯員数 = np.count_nonzero(np.isin(世帯員の学年一覧, [小学生学年.三年生.value,小学生学年.六年生.value,中学生学年.三年生.value, 高校生学年.三年生.value]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Openfisca推奨の書き方は以下の感じです。

小学三年生である = 世帯員の学年一覧 == 小学生学年.三年生.value  # 条件に該当するか否かのbool値のndarray配列ができる
小学六年生である = 世帯員の学年一覧 == 小学生学年.六年生.value
中学三年生である = 世帯員の学年一覧 == 中学生学年.三年生.value
高校三年生である = 世帯員の学年一覧 == 高校生学年.三年生.value
ボーナス対象学年である = 小学三年生である + 小学六年生である + 中学三年生である + 高校三年生である  # OpenFiscaでは or の代わりに + を用います
給付金額一覧 = 年齢が3の倍数である * (定数) + ボーナス対象学年である * (定数)  #  世帯員ごとに給付される額が格納されたndarray配列ができる
給付金額 = 対象世帯.sum(給付金額一覧)   # numpy の関数はできるだけ使わないことが望ましい

docs/tutorial.md Outdated
```

2. コンテナを再ビルドできたら、下記のcurlコマンドで `/calculate` にPOSTリクエストを送る
- レスポンスの3の倍数給付金に金額が入っていることを確認する
Copy link
Contributor

Choose a reason for hiding this comment

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

docker compose が起動した状態で http://localhost:8080 にアクセスするとSwagger-UIが使えるので、そちらのGUIでPOSTする方が簡単かもしれません。

docs/tutorial.md Outdated
"ETERNITY": "1979-01-01"
}
},
"子ども0": {
Copy link
Contributor

Choose a reason for hiding this comment

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

(細かいですが...)テストの表記に合わせて 子1 にすると分かりやすいかなと思いました。

Copy link
Contributor

@SnoozingJellyfish SnoozingJellyfish left a comment

Choose a reason for hiding this comment

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

修正ありがとうございます!
細かい点ですがいくつか追加コメントしています。
チュートリアルのファイルを作るとうっかりプッシュしてしまうかもしれないので、variables, parameters, testsのそれぞれチュートリアルフォルダ以下を .gitignore に追加してもいいかもしれません。

docs/tutorial.md Outdated

### 1. ボーナス金額のパラメーターファイルを作成する

1. 下記の内容で`parameters/チュートリアル/3の倍数給付金額_ボーナス.yaml` のファイルを作成する
Copy link
Contributor

Choose a reason for hiding this comment

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

parameters/チュートリアル/三の倍数給付金額_ボーナス.yaml (漢数字)の誤記だと思うので修正お願いします!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

別のターミナルで docker compose up --build を実行している状態でないと docker compose exec openfisca /bin/bash コマンドが多分実行できないので、その旨記載お願いします🙏

補足説明を追記しました。

parameters/チュートリアル/三の倍数給付金額_ボーナス.yaml (漢数字)の誤記だと思うので修正お願いします!

漢数字に修正しました。

@AZKZ
Copy link
Contributor Author

AZKZ commented Dec 26, 2023

@SnoozingJellyfish @Syuparn

ご確認いただきありがとうございます!
指摘いただいた内容を修正したので、お手好きの際にご確認をお願いします!

docs/index.md Outdated
@@ -23,4 +23,6 @@
- [Google_custom_search_APIについて](./chat_bot.md#google_custom_search_apiについて)
- [miiboについて](./chat_bot.md#miiboについて)

### [チュートリアル](./tutorial)
Copy link
Contributor

Choose a reason for hiding this comment

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

(前回のレビューで気づけなかったのですが 🙏 )ここのリンクが切れてしまっているので修正お願いします。

Suggested change
### [チュートリアル](./tutorial)
### [チュートリアル](./tutorial.md)

docs/index.md Outdated
@@ -23,4 +23,6 @@
- [Google_custom_search_APIについて](./chat_bot.md#google_custom_search_apiについて)
- [miiboについて](./chat_bot.md#miiboについて)

### [チュートリアル](./tutorial)

### [変更履歴 (Change log)](./change_log)
Copy link
Contributor

Choose a reason for hiding this comment

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

今回の改修と関係ないのですが、忘れないうちにこちらのリンク切れも修正いただけるとありがたいです。

Suggested change
### [変更履歴 (Change log)](./change_log)
### [変更履歴 (Change log)](./change_log.md)

@Syuparn
Copy link
Contributor

Syuparn commented Dec 26, 2023

@AZKZ
リンクが切れていた箇所を追加でコメントしましたのでご確認お願いします。それ以外は問題ないと思います!

@AZKZ
Copy link
Contributor Author

AZKZ commented Dec 27, 2023

@Syuparn

ご確認ありがとうございます!
Github Pages上では.mdはなくても画面遷移できるようでしたが、他と合わせるという意味でも修正いたしました!

@Syuparn
Copy link
Contributor

Syuparn commented Dec 27, 2023

修正ありがとうございます!内容良いと思います!

@SnoozingJellyfish
確認いただいて問題なければマージお願いします。

@SnoozingJellyfish
Copy link
Contributor

修正・レビューありがとうございます!確認できたのでマージします。
これでぐっとOpenFiscaに取り組みやすくなると思います!

@SnoozingJellyfish SnoozingJellyfish merged commit 67503e7 into project-inclusive:develop Dec 27, 2023
6 checks passed
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