Model refactoring stage 1: "evaluate" normalization#2580
Conversation
|
I like the way this is going. I did not go through all lines for how the inputs are prepared, transposed, etc. but I like the idea. |
|
This makes a lot of sense. One thing that occurs to me is that |
|
The one comment I have is whether it might not be better to give multiple inputs as one tuple instead of separately. This allows the model to be agnostic about the number of inputs, which can be very useful, e.g., when fitting a spectrum to a linear combination of an arbitrary number of bases (as might happen in spectral synthesis, or in determining a line broadening function (Rucinski 2002AJ....124.1746R), etc). One implementation would be to make this similar to what now seems to be done for outputs (where even a single output is turned into a tuple, so one can iterate over it). The thing I'm less sure about is whether a single input should just be passed on as is, or whether it should be a tuple too (probably the former is best). |
Currently that's just a reflection of the existing implementation. I'm reworking the exact details of how inputs/outputs are prepared in a side-branch. Still working out some of the details though. |
Part of the reason for splitting out The |
|
@mhvk That's a discussion worth having (and that has already been happening in other channels) but is mostly orthogonal to the main purpose of this PR, which is just one building block for future updates. At some point I think I am going to work on an APE or series of APEs to clarify all the details and provide full justification for various design decisions. That would be a good place to talk about details like that. |
|
@embray - I guess you are right that at the moment there is a relatively simple change of |
…ear fits only, which is the only place that is supported for now). Right now this only supports fitting with *one* input across all models (such as when evaluating the model with model_set_axis=False). This is actually in line with what was previously supported. The only major difference is that it's no longer to transpose the 'y' input before passing to the fitter. This is, as I see it, an internal detail of how the fitting algorithm works and not something users should be responsible for. I'll clean this up further once I've merged this work astropy#2580, which will make further improvements easier.
|
Rebased this to fully incorporate the work from #2634. This was a tad tricky but I think it will work. This was a necessary precursor to any further heavy duty work on models :< |
|
Great. Apparently my doc build went from one warning to two with that last commit. I'll check this out again tomorrow.... |
|
Updated with a link to the docs as well: http://embray.github.io/astropy/modeling/index.html |
|
@embray I'm getting the following modeling test failures with Python 2.7 on Mac: |
|
I think after rebasing this a few times Travis has given up updated the build status for this PR. However it is still doing the builds. The current build (for 282a0c8) is here: https://travis-ci.org/astropy/astropy/builds/31286649 |
|
Ok, FWIW the latest build passed. |
There was a problem hiding this comment.
Lines 171-194 need one level of indentation.
|
@cdeil I updated this PR to include more graceful deprecation for the eval -> evaluate switch (at least I think). I tested gammapy against this and all tests now pass. I'll check photutils as well shortly. |
|
Confirmed, all phoutils tests pass against this PR as well. |
|
@embray Thanks for keeping If we want the upcoming
|
There was a problem hiding this comment.
Can you make this a Python code-block so that the code is syntax highlighted?
There was a problem hiding this comment.
Oh, sure, I thought normally it did that automatically.
There was a problem hiding this comment.
In fact, I think normally it does, but it didn't in this case due to the syntax error. That said, I prefer the explicit code-block directive.
There was a problem hiding this comment.
Ah ... I was wondering why Python syntax highlighting didn't already work with the :: code block ... so Sphinx gives up if there's a SyntaxError without generating a warning? ... if yes, I think that's not very nice.
There was a problem hiding this comment.
I'm not exactly sure how it's handling :: blocks without a language specification. Because those blocks don't necessarily contain source code, much less Python source code (or whatever the default language is set to). So it might just try parsing it as the default language, and only highlighting if parsing succeeds (and treating it as plaintext otherwise).
|
Currently the example in the docs that explains how to implement a new model does override If there's an case more than a few users might run into there could be an advanced example further down that explains about |
|
@cdeil You're right--actually |
generic __call__, with all the machinery for evaluating a given model in a .evaluate() method that every model is required to have--preferably a staticmethod or a classmethod so that it can implemented easily independtly of the model class it belongs to. The new system is not yet implemented for composite models or for polynomial models, which will require some further refactoring.
…f implementing all its core calculation in an evaluate method. This includes prepare_input and prepare_output methods that can be overridden by subclasses and used also by fitters. The exact semantics of prepare_input and prepare_output I am not quite happy with yet--currently they do what is necessary to preserve the existing input handling, which I intend to rework. Finally, Model.param_sets is no longer an ndarray, but just a normal list (albeit of ndarrays). This was the only way I could get it to work consistently with multi-dimensional parameter values. I plan to replace Model.param_sets with something else later.
… mention of process_input/output but I think that's a pretty advanced thing most models won't need to touch). Fixed, I think, the documentation references that were failing
…ing to have whenever building the docs.
|
On second thought, rereading that section it already explains the only reason to override |
|
Without any further comments I'll go ahead and merge this I think. |
Model refactoring stage 1: "evaluate" normalization
This PR is to demonstrate one aspect of where I'm going with modeling refactoring. This PR is not entirely complete--there are some missing features, and a few models don't work with it (in particular composite models, though they are being refactored as well). This PR does not yet demonstrate the full advantage of the changes it presents. I just wanted to get it out there so interested parties could get an idea of where things are going.
The primary change demonstrated by this PR is as follows: Most model subclasses will no longer implement a
__call__method manually.Model.__call__is implemented in the base class, and is not generally overridden by subclasses. There's nothing stopping a subclass from overriding it, but I'm trying to move things a direction where it generally won't be necessary.Instead, when implementing a new model, developers/users must supply a method called
evaluate. This is almost completely analogous to the existing implementation ofFittableModel(formerlyParametricModel) subclasses that required implementing anevalmethod. (The name change is mostly justified by the fact that "eval" is a Python builtin, so using it confuses some syntax highlighters; also it doesn't need to be typed often so a little verbosity doesn't hurt).The
evaluatemethod may be either a normal instance method, aclassmethod, or astaticmethod. Regardless, it takes a fixed set of arguments, consisting of all of the function's independent variables, followed by the functional parameters needed to evaluate the function. The parameters are always provided in a fixed order based on the order they were defined on the model class. The arguments are always assumed to by Numpy arrays that are already in the correct shape to be evaluated. The key difference here is that all models are implemented this way, not just "fittable models", which may eventually be removed as a separate class. The goal is to reduce the number of different ways of implementing the model, as well as the amount of boilerplate needed.One advantage of this is that it makes it easy to swap out implementations of
evaluatefor any given model. As long as the implementation is wrapped to take arguments in the correct order it could be a Numpy implementation (like we currently use), or a function using numexpr, or written in C or Cython, or what have you. A related advantage, not yet demonstrated by this PR, is that it will make some aspects of implementing fitters simpler and, I think, more efficient. I am working on another set of changes on top of both this and #2441 that I hope will demonstrate the full advantages.Update: You can view the docs for this PR here: http://embray.github.io/astropy/modeling/index.html