lnccbrown / HSSM

Development of HSSM package
Other
82 stars 11 forks source link

Plot improvements #598

Closed AlexanderFengler closed 1 day ago

AlexanderFengler commented 1 month ago

PR for plot improvements.

AlexanderFengler commented 3 weeks ago

This is still work in progress, roughly 80% done.

cpaniaguam commented 1 week ago

@AlexanderFengler What do you think about creating a PR (ideally a few given its size) for model_cartoon.py? I think this will facilitate reviewing.

AlexanderFengler commented 1 week ago

@AlexanderFengler What do you think about creating a PR (ideally a few given its size) for model_cartoon.py? I think this will facilitate reviewing.

Sorry I know this is a big PR, but I really can't go back an disentangle this easily now. Will avoid (and generally do) big PRs like this. Changes are all interrelated I can't make a PR out of only the changes to model_cartoon.py here, it would take a lot of effort.

model_cartoon.py is new and the major piece, the rest is just a bunch of refactor, some little utilities, and minor subtle bug fixes in the other plotting functions (me looking at those closely was overdue).

The ++ line count is also inflated by having added two data files, so it's not as bad as it seems on first glance :).

AlexanderFengler commented 4 days ago

Approved in principle. I see that there are some unresolved comments in model_cartoon.py. Is that because GitHub hides the file by default due to size?

hm will give it another look, but couldn't find any more unaddressed comments las time I checked.

codecov[bot] commented 4 days ago

Codecov Report

Attention: Patch coverage is 86.19469% with 78 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/hssm/plotting/model_cartoon.py 91.24% 38 Missing :warning:
src/hssm/plotting/utils.py 26.92% 19 Missing :warning:
src/hssm/plotting/posterior_predictive.py 58.53% 17 Missing :warning:
tests/test_plotting_cartoon.py 89.47% 4 Missing :warning:
Files with missing lines Coverage Δ
src/hssm/plotting/__init__.py 100.00% <100.00%> (ø)
src/hssm/plotting/quantile_probability.py 92.22% <100.00%> (ø)
tests/conftest.py 93.50% <100.00%> (ø)
tests/test_plotting.py 99.31% <100.00%> (ø)
tests/test_plotting_cartoon.py 89.47% <89.47%> (ø)
src/hssm/plotting/posterior_predictive.py 78.94% <58.53%> (ø)
src/hssm/plotting/utils.py 73.33% <26.92%> (ø)
src/hssm/plotting/model_cartoon.py 91.24% <91.24%> (ø)
AlexanderFengler commented 3 days ago

@digicosmos86 ok to merge?

digicosmos86 commented 2 days ago

@digicosmos86 ok to merge?

I still see some comments in model_cartoon.py that are not resolved. Not sure if you saw them or not since large diffs are not rendered on Github by default and you have to manually load them. I've approved none-the-less so OK with me on my end