search

meyer-lab / tHMM

A general Python framework for using hidden Markov models on binary trees or cell lineage trees.
https://asmlab.org
MIT License
10 stars 1 forks source link

figure edits for review #857

Closed jclagarde closed 3 years ago

jclagarde commented 3 years ago
Screen Shot 2021-10-26 at 5 48 54 PM Screen Shot 2021-10-20 at 5 50 01 PM

So for line 108 and 112 for figure4.py, I was getting an error that had to do with the spelling of censore I had changed it and it resolved that specific issue but it led to more errors

Screen Shot 2021-10-26 at 5 52 40 PM

My main confusion is this error because cell.left is defined already within the CellVar class

codecov[bot] commented 3 years ago

Codecov Report

Merging #857 (9d329cd) into master (f0e91fd) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #857   +/-   ##
=======================================
  Coverage   74.15%   74.15%           
=======================================
  Files          24       24           
  Lines        1915     1915           
=======================================
  Hits         1420     1420           
  Misses        495      495
Flag Coverage Δ
unittests 74.15% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lineage/CellVar.py 97.43% <ø> (ø)
lineage/plotTree.py 0.00% <ø> (ø)

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 f0e91fd...9d329cd. Read the comment docs.

jclagarde commented 3 years ago

Are you trying to recreate figure4?

I was trying to recreate figure 3 in the manuscript, which I believe corresponds to figure4.py

Farnazmdi commented 3 years ago

okay, it should be similar to the problem we had with LineageTree, and it is because now the mypy is checking things. I will try some things and let you know.

Farnazmdi commented 3 years ago

Please do git pull and try now. It built for me.

jclagarde commented 3 years ago

Please do git pull and try now. It built for me.

It built for me as well. Thank you for fixing that issue!

jclagarde commented 3 years ago

The figure edits regarding issue #857 should be done now. Please take a look at them when you get the chance and let me know if there's any edits I should make. Thank you! :)

jclagarde commented 3 years ago
Screen Shot 2021-10-28 at 9 17 04 AM

I re-ran figure 4 and this is what the font looks like for both in the output folder on github and my VS code. Does it still look differently for you?

Farnazmdi commented 3 years ago

Thanks, okay, if it shows right for you, then the problem should be my browser or something. For the figure, could you run it a couple of times more? Also, could you temporarily (just to make the figure cartoon) change the color of the states to blue and green as it is shown here? I am looking for a tree that has a good number of cells in it. The one you have now, there are few cells in it. image

jclagarde commented 3 years ago

Thanks, okay, if it shows right for you, then the problem should be my browser or something. For the figure, could you run it a couple of times more? Also, could you temporarily (just to make the figure cartoon) change the color of the states to blue and green as it is shown here? I am looking for a tree that has a good number of cells in it. The one you have now, there are few cells in it. image

So I've ran figure 4 about 20 times now and I haven't been able to get anywhere near the amount of cells in that photo. Do you think there was a change in the code or should I just keep running it?

Farnazmdi commented 3 years ago

I hadn't seen your message here, JC. I don't think anything has changed. I suggest pick the best one you see among those.

jclagarde commented 3 years ago

I hadn't seen your message here, JC. I don't think anything has changed. I suggest pick the best one you see among those.

I'm experiencing similar merge issues as the last time where the easiest solution was to just create a new branch. Since I didn't edit too many lines of code, I'll just go ahead and copy these changes onto a new branch

jclagarde commented 3 years ago

I was able to resolve the merge issue without creating a new branch and I remade a figure 4 with a larger number of cells. Please take a look at it when you have the chance and let me know if I should make any further edits. Thank you! :)

Farnazmdi commented 3 years ago

Thanks JC, figure 4 looks great now.

jclagarde commented 3 years ago

Wait, In figureS8-S10 the y_label and title are the same. THey should not be. Please edit this.

image

also now that I look at it, should I also remove the light blue and light green lines?

Farnazmdi commented 3 years ago

Those are the true values, should be there for comparison. But you can limit the yaxis limits of (a) and (e) from 0-1 to 0.5-1, and 0-15 for (c), (d), (f), (g).

jclagarde commented 3 years ago

Those are the true values, should be there for comparison. But you can limit the yaxis limits of (a) and (e) from 0-1 to 0.5-1, and 0-15 for (c), (d), (f), (g).

Just edited the y-axis limits and labels. Please let me know if this is what you had in mind when you get the chance

Farnazmdi commented 3 years ago

Yes thank you. I will have to close this PR as I integrated your edits into my PR. Great work, thank you.

jclagarde commented 3 years ago

Yes thank you. I will have to close this PR as I integrated your edits into my PR. Great work, thank you.

Sounds good, thank you!