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

Support Problem Constants Injection #54

Merged
merged 20 commits into from
Jan 4, 2019
Merged

Support Problem Constants Injection #54

merged 20 commits into from
Jan 4, 2019

Conversation

kyuridenamida
Copy link
Owner

@kyuridenamida kyuridenamida commented Jan 2, 2019

MODとかYES, NOをコードに差し込む

存在していれば挿入というロジックのために今より高度なテンプレートエンジンが必要だったので、Jinja2を導入した
その際、下位互換性を持たせるために"${"が文字列に含まれていたら古いテンプレートエンジンを使うようにした。

対応した定数は以下の通り

  • mod MOD値
  • yes_str Yes or Noを出力する問題でのYesのための文字列
  • no_str Yes or Noを出力する問題でのNoのための文字列

どうせ間違ってたら使われないだけなのでかなり適当なのでRecallが重要だと思う。
とりあえずMVPということRecallは定量的にできていない。

新しいテンプレートエンジン(Jinja2)を用いたテンプレート記述例

#include <bits/stdc++.h>
using namespace std;

{% if mod %}const int mod = {{ mod }};{% endif %}
{% if yes_str %}const string YES = "{{ yes_str }}";{% endif %}
{% if no_str %}const string NO = "{{ no_str }}";{% endif %}

void solve({{formal_arguments}}){

}

int main(){
    {{input_part}}
    solve({{actual_arguments}});
    return 0;
}

とか書くと生成してくれる。

@codecov-io
Copy link

codecov-io commented Jan 2, 2019

Codecov Report

Merging #54 into master will increase coverage by 0.29%.
The diff coverage is 93.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
+ Coverage   91.07%   91.36%   +0.29%     
==========================================
  Files          32       34       +2     
  Lines        1579     1668      +89     
==========================================
+ Hits         1438     1524      +86     
- Misses        141      144       +3
Impacted Files Coverage Δ
atcodertools/codegen/template_engine.py 100% <100%> (ø) ⬆️
atcodertools/tools/envgen.py 81.81% <100%> (+0.23%) ⬆️
atcodertools/fileutils/create_contest_file.py 100% <100%> (ø) ⬆️
...odertools/models/constpred/problem_constant_set.py 100% <100%> (ø)
atcodertools/codegen/code_generator.py 88.88% <100%> (+1.38%) ⬆️
atcodertools/codegen/cpp_code_generator.py 94.28% <100%> (+0.05%) ⬆️
atcodertools/models/problem_content.py 95.23% <81.81%> (+2.73%) ⬆️
...codertools/constprediction/constants_prediction.py 92.3% <92.3%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e67a223...de22982. Read the comment docs.

@kyuridenamida
Copy link
Owner Author

テストが重いのでAGCに対してしか実行していませんが、検出状況はこんな感じです。
https://github.com/kyuridenamida/atcoder-tools/pull/54/files#diff-ff3e9b33f0682b2697a42e773baa07bf

@kyuridenamida
Copy link
Owner Author

あと、Jinja2は十分強力なので--replacementと--templateの2つを指定しなくなって使いやすくなる気がします。このPRにはその廃止は含まれていませんが。

@kyuridenamida kyuridenamida added this to the 1.0.5 milestone Jan 2, 2019
@asi1024
Copy link
Collaborator

asi1024 commented Jan 2, 2019

@kyuridenamida コンフリクト解消して

@kyuridenamida
Copy link
Owner Author

@asi1024 わかった まって

@kyuridenamida
Copy link
Owner Author

kyuridenamida commented Jan 2, 2019

した。あとは

  • formal_arguments -> parameters
  • actual_arguments -> arguments

にする作業をする

@asi1024
Copy link
Collaborator

asi1024 commented Jan 2, 2019

名前変えるのは別PRにしたほうがいいかも?

@kyuridenamida
Copy link
Owner Author

↑たしかに ではこのPRでは行いません

atcodertools/constprediction/constants_prediction.py Outdated Show resolved Hide resolved
atcodertools/constprediction/constants_prediction.py Outdated Show resolved Hide resolved
atcodertools/constprediction/constants_prediction.py Outdated Show resolved Hide resolved
atcodertools/models/problem_content.py Outdated Show resolved Hide resolved
atcodertools/models/problem_content.py Outdated Show resolved Hide resolved
atcodertools/models/problem_content.py Outdated Show resolved Hide resolved
atcodertools/codegen/template_engine.py Outdated Show resolved Hide resolved
asi1024 and others added 7 commits January 3, 2019 00:30
Co-Authored-By: kyuridenamida <tyotyo3@gmail.com>
Co-Authored-By: kyuridenamida <tyotyo3@gmail.com>
Co-Authored-By: kyuridenamida <tyotyo3@gmail.com>
…ndidates. Instead, show warning message when detecting multiple values returning None.
return None


def predict_yes_no(html: str) -> Optional[Tuple[str, str]]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

型が実装と合ってなくないですか?

Copy link
Owner Author

@kyuridenamida kyuridenamida Jan 3, 2019

Choose a reason for hiding this comment

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

確かにOptional[Tuple[Optional[str], Optional[str]]]でしたね。うっかりしてました。
Noneに失敗の意味を持たせるのもあまり好きじゃないので、Optionalの代わりにエラー吐くようにしました。

なので型が

def predict_yes_no(html: str) -> Tuple[Optional[str], Optional[str]]:

になった。

Copy link
Collaborator

@asi1024 asi1024 Jan 3, 2019

Choose a reason for hiding this comment

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

yes_str が定義されているとき,その時に限り no_str が定義されているというのは保証してほしいです (型を Optional[Tuple[...]] にしてほしいというのはそういう意図でした)

Copy link
Owner Author

Choose a reason for hiding this comment

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

解があるなら普通に出力、ないならImpossibleを出力するみたいな問題を想定した上で今の実装にしていました(no_strだけが存在するケース)。
ただ、その反対のパターンつまりyes_strだけが存在するケースってのは多分想像つかないので型としてはちょっと緩いですが実用上問題ないというつもりでいます。
どう思いますか?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No と Impossible を同じ no_str に入れるのは微妙な気がしますが,どうなんでしょう

Copy link
Collaborator

Choose a reason for hiding this comment

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

ひとまずはこれでいっか

@@ -1,13 +1,16 @@
#include <bits/stdc++.h>
using namespace std;

void solve(${formal_arguments}){

{% if mod %}const long long MOD = {{ mod }};{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

この if は C でいう #ifdef 的なやつですよね. mod 自体は bool 値ではないので,わかりづらい気がします.

Copy link
Owner Author

Choose a reason for hiding this comment

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

そうですね。pythonのif文と同じ挙動をします。てことはやっぱり明示的に mod is not Noneって書いたほうがいいですね

Copy link
Owner Author

Choose a reason for hiding this comment

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

同じ挙動をするというのは嘘で、jinja2独自の動作をするが正しかったです。どちらにせよ none かどうかを明示的に判定したほうがよいのはそうっぽいのでそうします。

tests/test_constpred.py Show resolved Hide resolved
@asi1024
Copy link
Collaborator

asi1024 commented Jan 3, 2019

{% if ... %}{% endif %} はネストできますか?できるのならばテストにそういうケースを追加してほしいです.できないならサポートしてほしいです.

@kyuridenamida
Copy link
Owner Author

@asi1024 jinja2の機能としてはできるはずです。そのテストを追加しますね。

@kyuridenamida kyuridenamida self-assigned this Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants