-
-
Notifications
You must be signed in to change notification settings - Fork 396
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
Unify model comparison api #1690
Unify model comparison api #1690
Conversation
I haven't created any tests yet because I have a little trouble understanding all the parameters in the compare tests |
@OriolAbril could you please explain to me what I have to test in the compare function? |
Something along the lines of |
Thank you for your help! Of course and I can do that, great idea! Although where should I store this helper function? Should I store it somewhere separately and import it to both compare and elpd_plot or store it in every python file it is used? |
Created the Helper function Calculate_ICS, although I haven't run any tests to see if it works yet. Also, I am not sure if it is wrong that I don't return the |
The function should be in a separate file, maybe in |
…ub.com/alexisperakis/arviz into development/Unify_model_comparison_api
Hello @oriol! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have difficulty running the tests I created for my function Calculate_ICS stored in the arivz.utils directory.
The problems might be due to the some of the comments like the wrong number of return arguments to assign or the circular import. The function itself looks great though, it simplifies the code while extending functionality, thanks!
I can not understand what are the input values I can give the compare_dict or the dataset_dict in order to test my helper function.
I think it will already be tested via the tests for compare and plot_elpd, so before working on tests, I would first get the current tests to run to see the coverage, and then see if we can for example simplify two tests on compare and plot_elpd to a single test on calculate_ics or if we need to add some specific calculate_ics tests in addition to the existing ones.
@galexopoulos I posted in issue #1643 some code but it looks like you are further along here than I was. Are you still working on this? Looks like mostly missing tests at this point? |
b6b47a0
to
dc3f2e1
Compare
@derekpowell can you test the PR? It should be working for both compare and plot_elpd taking dicts of inferencedata, of elpddata or mixture of both. You can install arviz from this PR directly using https://stackoverflow.com/a/50095199/2504700 |
Codecov Report
@@ Coverage Diff @@
## main #1690 +/- ##
==========================================
- Coverage 90.84% 90.82% -0.03%
==========================================
Files 114 114
Lines 12327 12355 +28
==========================================
+ Hits 11199 11221 +22
- Misses 1128 1134 +6
Continue to review full report at Codecov.
|
Thanks for the PR |
Description
Checklist