Separate optimization from statistic redux#2441
Conversation
|
@embray - it looks like the sphinx failures are genuine. |
|
I fixed a few of the problems with the doc build, but the issue remains as to what to do with the "what's new" page. I raised this question the mailing list. |
|
Rebased this to get Travis to build once more against the latest master, where the problem with the "what's new" doc page should be moot. |
|
Assuming this passes, finally, I hope to get it merged. Current work I'm doing on the modeling package depends on this work (albeit none of it is necessary for 0.4). |
|
@eteq - I'm not involved in this (though it certainly seems a good idea, so in that sense an uninformed +1). |
|
This is basically ready to go as far as I'm concerned. I'm working currently on a few improvements on top of this but they won't be ready in time for 0.4 and won't be significant changes to the overall structure. So I think this is better to have now than not. Though I'm happy to update the PR if anyone would like to review this and point out any glaring issues--in fact it would be appreciated. |
There was a problem hiding this comment.
I think a user-defined fit statistic is quite common.
So it shouldn't be necessary to import functions with underscores (i.e. private implementation details).
How about making those functions public by removing the underscores and include them in the docs (maybe in this part of the docs)?
There was a problem hiding this comment.
Oops ... I made the same comments here and you disagreed: #1878 (comment)
Note that e.g. in Sherpa this is an FAQ: http://cxc.harvard.edu/sherpa/faq/user_stat.html
There was a problem hiding this comment.
I agree, and am working on refactoring how that's done. In the meantime though this basically documents how they existing code is implemented so that other developers can copy it. I'm working on simplifying things so that fewer "internal" details need to be worried about, but that will have to wait til later.
There was a problem hiding this comment.
Well, I disagreed with you there, but I agree with this in spirit. It's just not a high priority and not an issue for how these docs are currently to be read or who the intended audience is :)
There was a problem hiding this comment.
Oh yeah, in my refactor of this the custom statistic functions are just as straightforward to implement as in Sherpa. For example, this is what least_square looks like (in the current in development version, so far):
def least_square(measured_values, approximated_values, weights=None):
return np.sum(residuals(measured_values, approximated_values, weights=weights) ** 2)
(where residuals is defined in a similarly obvious way). I should note that the arguments currently accepted by this function are not what general functions are limited to. Custom statistics will be able to accept additional arguments as needed, such as uncertainties.
|
I have one comment on I tried improving this in #1878 which was never merged (mostly my fault) and by now would probably have to re-done from scratch. |
|
@cdeil Do you think it would help if, at the end of the docs, a "full example" were pieced together from the previous text? |
|
IMO the best way would be to write such long examples is directly as "full example" (that's what I did in #1878 for this) so that the user can run the example and it can be run as part of the tests. If you don't want to make this change to the docs, how about putting the full example as a unit test (still has to be manually updated in two places, but at least it's tested). |
|
If I understand correctly, that would involve writing any explanation in the comments of the example. I have to problem with that, I just want to make sure I understand correctly. |
|
Writing explanations in the comments of the example is one option. Another one is writing the comments before and / or after the full example. |
There was a problem hiding this comment.
This new class is currently not covered by tests ... can you add at least one?
There was a problem hiding this comment.
Turns out it is covered by the test test_fitter_against_LevMar, but I'll add another explicitly for that fitter.
|
I've left some inline comments ... mainly asking for a test that covers the new code. I can try this out tomorrow (it would be nice if there were working examples in the docs I can just run). |
There was a problem hiding this comment.
objective_function : callable
Objective function
|
I'm not so sure about that. Having the explanation not inline with the code means repeated looking up and down the page and trying to match up the explanation with the particular line of code. That's a real pain. One could open two copies of the page side-by-side but that's not always feasible either. |
There was a problem hiding this comment.
Add a dot . at the end of this sentence.
The Parameters section in the Sphinx output is currently garbled because of this (check the html output).
IMO a code example I can actually run is much more important than where the explanations are placed. But I'll shut up about that now. |
|
@cdeil I'm probably going to hold off on going too crazy right now on the examples in the documentation, though I agree with you on all this in general. Reason being (and I hate to keep having to say this) I'm working on an overhaul of how the optimizer/statistic separation is implemented. The basic idea is still the same but I'm trying to simplify things a little. I would go ahead and append my work on that to this PR, but it depends on #2580 (and isn't quite yet finished). All that said, should we maybe hold off on this entirely until my refactoring is ready or is it good to go ahead and pull this in now? |
I think the sooner your re-factorings go into Let me know if I can help with reviewing some PR or if there's something I could maybe implement. |
…taldcroft that these examples expose an unfortunate degree of complexity involved in creating new Fitters, such that it is not really at a point where casual users can do it. But these docs are still useful to have for developers to look at--in the future I hope to try to hide more of the sausage-making if I can. Also redid the section headings per the preferred formatting.
…oing through a 'utils' module. Use the NumpyRNGContext where appropriate.
…out by @cdeil in astropy#2441, but also including several other mistakes I found while reading through the docs.
There was a problem hiding this comment.
This gives a sphinx warning and makes the travis-ci build fail:
https://travis-ci.org/astropy/astropy/jobs/26970976#L3333
There was a problem hiding this comment.
Ack. I thought I saw that in the logs too but I couldn't find where it was coming from. Incidentally, I removed this already but in a different branch (the one where I'm refactoring) =_=
…r=True to a couple models that are clearly linear in their parameters
|
Tests passing now. If there are no further comments this should be merged, with the understanding that there will continue to be improvements here shortly after 0.4 is out, I hope. |
|
Sounds good! Thanks for all your work on this! |
|
Thanks to @nden for the lion's share of the work. |
Separate optimization from statistic redux
This is a rebased version of #1914. I decided to go ahead and make a new PR so that it can easily be run through Travis and the like. Otherwise no major differences. Once the CI gives us a green checkmark this should be merged instead of #1914.