-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
added akaike and bayesian information criterion and documentation #3700
added akaike and bayesian information criterion and documentation #3700
Conversation
Hi @astronomeralex thanks for contributing this! Can I ask you to add some tests as well? It would be helpful to at least add one test for each tasks that test whether the correct value is returned for a given set of values. It also would be helpful to test that the code properly handles if the code can correctly handle the different types of the variables passed to it (single floats or arrays ). And it would appear that the build is currently failing on
where it isn't sure if num_params is an array or float. This would be a good example of something that would need a test. |
@crawfordsm right, yeah, num_params can be an array or a float. i'll fix that and add some tests today. |
@crawfordsm -- after fighting tests and typos, i think this is ready to merge? |
203c869
to
e66763d
Compare
def test_akaike_information_criterion(): | ||
#testing with arrays | ||
aicc = funcs.akaike_information_criterion(np.array([-10,-5]), np.array([2,3]), 20, True) | ||
assert_allclose(aicc, np.array([ 24.70588235, 17.5])) |
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.
Not to be paranoid, but where to the numbers 24.70588 ... and 17.5 come from? Do you know that this example is true (e.g. from a trustworthy reference) or did you calculate them with your new code, in other words, were they calculated assuming the code is right? (In that case, the test still has some value because it prevents bugs that could will be introduced through code changes in the future, but obviously it would be best to test a "known good" example to make sure that the current implementation is right.)
I wonder about how this could be added to astropy.modeling. For example, could we add this to the fit_info attached to some fitters? Would these even be useful for any of the existing fitters? I don't know the math here so I don't know the answers to these questions. |
@astronomeralex - could you implement the fixes suggested by @hamogu, @adrn, and @embray? @embray - I agree we should think about this, but this should not hold back this PR. |
@astronomeralex Any time to update this PR. Failing that, is there anyone who can be assigned to it to ferry it through? |
@astronomeralex just pinging you on this to see if you have some time to finalize this? |
If @astronomeralex does not have time to finish this, and if you guys think this is still relevant whatsoever, I would like to be assigned to finish it up. |
@mirca if you want to work on this next, that would be helpful. |
Closing this after #4716 was merged. |
I've added two common information criterion in astro, with an example for each and a reference to a paper where they're discussed.