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

solved #21 #31

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

hamazaki1990
Copy link
Contributor

できたと思います。
・適応度を遺伝子型に応じて持たせたくなると思ったので、next_genwfなどをいじると混乱しそうだったので新しい関数を作りました。出題の意図に沿えてなかったらすみません。
・関数や変数で分かりやすい名前をつけようとすると、一行の文字数が長くなってしまう(lintarに怒られる)のですがどうしたらいいものでしょうか
・list.sort()がなぜか使えなかったのですが、ライブラリのインポートが必要なメソッドなのでしょうか?

@heavywatal
Copy link
Owner

別の関数を作るということに関しては問題ないです。が、集団中の全個体に一気に突然変異を起こさせるというアプローチはちょいと過激です。複製するときにある確率で、という自然な感じでお願いします。

v1.sort() は in-place な処理なので v1 そのものがソートされ、返り値はありません。オリジナルを維持しつつ、ソート済みのコピーを得るには v2 = sorted(v1) のようにします。

1行の文字数を抑える方法にはいくつかありますが、とりあえず大きく2つ:

  • 適宜改行する。ただしpep8/flake8に怒られないようにインデントを整える必要がある。
  • こまめに変数を作る。

あとこの課題は #22 です。

些細な点:

  • 突然変異率を個体ごとに変えることはそうそう無いので self._mutationrate を各個体に持たせる必要はありません。クラス変数にするか individual.py のグローバル変数にしておくのが良いでしょう。
  • 数値の大小を比較するときは if x < y: のように小さい方を左にするか、 if x > some_constant: のように変数を左(つまり定数を右)に書くほうが読みやすいです。
  • return を2行連続で書いても、最初のを実行した時点で関数を抜けてしまうので2つめは意味なくなります。
  • get_***() という名前の関数は、既に持ってるオブジェクトを返すだけとか、一瞬で終わることが期待されるので、複雑な計算が含まれる関数には別の名前をつけたほうがいいです。
  • for文は range() で整数を回すだけじゃなく、リストを直接参照できます:
for i in range(len(lst)):
    do_something(lst[i])

for x in lst:
    do_something(x)

ちなみにこういうループの整数カウンタには ij を使うのが一般的です。

@hamazaki1990
Copy link
Contributor Author

・クラス変数としてmutationrate = 0.01、などとおいてしまうと遺伝子によって変異率を変えたい時に不便な気がしますが、mutationrate = muとしてさらに初期化関数でmu = 0.01などと書き換えるのは良くないような気がして詰まっています(多分クラス変数についてあまりよくわかっていません、、、)
・population.pyの54-56行目は内包表記できそうな気がしたのですがエラーが出ました。(シーケンス内のシーケンス追加だとダメなのでしょうか?)

@heavywatal
Copy link
Owner

  • 前者は先のことを心配しすぎ、後者はクラス変数の意味がないですね。何あたりの突然変異になるかという量的な議論とかもまあ置いといて、とりあえず次の図のような感じのことが観察できるようになったらいいです。ああ、これまでの課題の成果として、シミュレーションのデータからこういう図を描いて提出してもらえたら最高です。
  • どう書いてどういうエラーが出たか分かりませんが、この用途なら私の知る限り extend() 一択で内包表記は使わないと思います。

genetic-drift-with-new-mutation

@hamazaki1990
Copy link
Contributor Author

population.pyを書き換えてみたのですが、例えばある世代が[1,2,3,3,2]で、2番目の2に変異を起こしたいときに、2番目にも5番目にも同じ変異が入る仕様になってしまいます。roulettechoiceではpopulationsのi番目、という形で指定しているのにそうなる理由がわかりません

@heavywatal
Copy link
Owner

「浅いコピー」の問題ですね。誰もが通るハマりポイントで、ネットにわんさか情報があるのでじっくり理解してください。

@hamazaki1990
Copy link
Contributor Author

グラフのためのCSVの書き出しまではできたつもりなのですが(visualize.py)、できたCSVをR studioで読み込もうとするとエラーがたくさん出ます

@heavywatal
Copy link
Owner

Wright-FisherでもMoranでも self._inds をまるまる deepcopy() するのはちょっと乱暴です。次世代に子供を残すことが確定した個体だけ複製するようにしましょう。例えば、コピーを返すメソッドを Individual にもたせて、その中でついでにmutationも起こしちゃうのがいいんじゃないでしょうか。

列の名前や数が実行するたびに変わるデータを扱うのは難しいです。一方、行の変化に対応するのは簡単です。例えば下のような形式のほうがtidyで扱いやすそうですし、 visualize.py での書き込み前の処理も楽になりそうです。

step,locus,frequency
# 略
8,0,0.02
8,1,0.40
9,0,0.08
9,1,0.30
# 略

私の手元で試してみたところ、読み込みでエラーは出ませんでした。このように、エラーには再現性が無い場合も多いので、報告と質問はもう少し具体的にしてください。どんな環境で、どんな変数に対して、どんなコードを実行して、どんな出力やエラーが出たのか、というのがなるべく分かるようにコンソールの内容を全部コピペしてここに張ってもらえると楽です。長くなっても構いません。

ここでソースコードっぽく表示させるには、3連back-tickで挟んだ fenced code block というを使います。言語指定すると色も付けてくれます。Markdown記法は今後もよく使うことになると思うので、早めに慣れていきましょう。
https://guides.github.com/features/mastering-markdown/

@hamazaki1990
Copy link
Contributor Author

できました!

@heavywatal
Copy link
Owner

  • acquire_mutation() が名前と異なる挙動なので危険。さらに next_ind._genotype = *** のように self じゃないインスタンスのプライベート変数をいじるのもあまり良くないです。 acquire_mutation() は自分に突然変異を起こすことに専念して、それを呼びつつコピーを返すメソッドは別に作りましょう。あるいはコピーメソッドを作らずにクラス外でコピーを作って、そいつで変異の関数を実行する、というのでもいいです。
  • for x in range(generation):x じゃなくて直接 t を使うほうが素直でわかりやすい気がします。
  • change_allele get_step() など名前の意味がちょっとよく分かりません。
  • Wright-Fisherに比べてMoranのほうがいい感じに見えないのはなぜでしょうか。プログラムはたぶん正しいので、意味と見せ方を考えてみましょう。
  • 今度から ggsave() で作ったPNGをコメント時に添付してください。
  • リンターを使わないとはいえRのコードがちょっと読みにくいので、空行やスペースをいい感じに使いましょう。

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.

2 participants