Models subpackage #493

Merged
merged 76 commits into from Apr 23, 2013

Conversation

Projects
None yet
@nden
Contributor

nden commented Nov 19, 2012

This PR is for the initial import of models and fitting routines.
The documentation is here:

https://github.com/nden/astropy/tree/master/docs/models

I have included some of the tests that I was using during development. Various parts of this package were tested against iraf, pywcs and matlab but not all tests are included here (some of the data files are large).

@eteq

This comment has been minimized.

Show comment Hide comment
@eteq

eteq Nov 19, 2012

Owner

For those who might not be aware, the idea to add this work to astropy came out of a discussion at the coordination meeting, and is a necessary step for the "generalized" WCS project. But It also should be very useful as a model-fitting interface that can be shared throughout astropy.

Owner

eteq commented Nov 19, 2012

For those who might not be aware, the idea to add this work to astropy came out of a discussion at the coordination meeting, and is a necessary step for the "generalized" WCS project. But It also should be very useful as a model-fitting interface that can be shared throughout astropy.

@embray

View changes

astropy/models/__init__.py
+import parameters
+import projections
+import rotations
+

This comment has been minimized.

Show comment Hide comment
@embray

embray Nov 20, 2012

Member

These should all be relative imports such as from . import models

@embray

embray Nov 20, 2012

Member

These should all be relative imports such as from . import models

@embray

View changes

astropy/models/fitting.py
+ 'singular_values': None,
+ 'pars': None
+ }
+ Fitter.__init__(self, model, fixed=fixed)

This comment has been minimized.

Show comment Hide comment
@embray

embray Nov 20, 2012

Member

Although not as important in single-inheritance cases, please still try to use super() instead. For example super(Fitter, self).__init__(model, fixed=fixed). See the seventh bullet point under http://astropy.readthedocs.org/en/latest/development/codeguide.html#coding-style-conventions

@embray

embray Nov 20, 2012

Member

Although not as important in single-inheritance cases, please still try to use super() instead. For example super(Fitter, self).__init__(model, fixed=fixed). See the seventh bullet point under http://astropy.readthedocs.org/en/latest/development/codeguide.html#coding-style-conventions

This comment has been minimized.

Show comment Hide comment
@nden

nden Nov 20, 2012

Contributor

OK, I'll change this.

@nden

nden Nov 20, 2012

Contributor

OK, I'll change this.

@astrofrog

View changes

astropy/models/fitting.py
+import abc
+import numpy as np
+from numpy import linalg
+from scipy import optimize

This comment has been minimized.

Show comment Hide comment
@astrofrog

astrofrog Nov 20, 2012

Owner

scipy is not a required dependency for astropy, so it should be imported only in the functions/methods that require it.

@astrofrog

astrofrog Nov 20, 2012

Owner

scipy is not a required dependency for astropy, so it should be imported only in the functions/methods that require it.

This comment has been minimized.

Show comment Hide comment
@nden

nden Nov 20, 2012

Contributor

Forgot about that, will make the changes

@nden

nden Nov 20, 2012

Contributor

Forgot about that, will make the changes

@astrofrog

View changes

astropy/models/fitting.py
+in scipy.optimize.
+
+"""
+from __future__ import division

This comment has been minimized.

Show comment Hide comment
@astrofrog

astrofrog Nov 20, 2012

Owner

For future-proofness, it would be best to also import print_function and change the print statements to the Python 3 syntax print().

@astrofrog

astrofrog Nov 20, 2012

Owner

For future-proofness, it would be best to also import print_function and change the print statements to the Python 3 syntax print().

This comment has been minimized.

Show comment Hide comment
@nden

nden Nov 20, 2012

Contributor

Good point! I actually haven't done much with python 3 yet. So I installed it and ran the tests. I'll change the print statements but will have to figure out why some tests are failing.

@nden

nden Nov 20, 2012

Contributor

Good point! I actually haven't done much with python 3 yet. So I installed it and ran the tests. I'll change the print statements but will have to figure out why some tests are failing.

@astrofrog astrofrog referenced this pull request in astrofrog/astropy-api Dec 2, 2012

Merged

Photometry API proposal #2

@astrofrog

This comment has been minimized.

Show comment Hide comment
@astrofrog

astrofrog Dec 8, 2012

Owner

@nden - just for information, I do plan to take a careful look at this, but it will be easier once 0.2 is out of the door. But at first glance, this looks great! I wonder whether it would already be worth isolating the WCS-specific stuff to astropy.models.wcs for clarity?

Owner

astrofrog commented Dec 8, 2012

@nden - just for information, I do plan to take a careful look at this, but it will be easier once 0.2 is out of the door. But at first glance, this looks great! I wonder whether it would already be worth isolating the WCS-specific stuff to astropy.models.wcs for clarity?

@nden

This comment has been minimized.

Show comment Hide comment
@nden

nden Dec 9, 2012

Contributor

@astrofrog - I am going to push changes to master soon - mostly related to moving constraints from fitters to models, you may want to wait for this.
The only strictly WCS-specific modules so far are projections.py and rotations.py but models in models.py can be useful for WCS too. Since init.py will import all modules from models and models.wcs, then I'm OK with splitting them, since I won't have to remember where a specific module lives.

Contributor

nden commented Dec 9, 2012

@astrofrog - I am going to push changes to master soon - mostly related to moving constraints from fitters to models, you may want to wait for this.
The only strictly WCS-specific modules so far are projections.py and rotations.py but models in models.py can be useful for WCS too. Since init.py will import all modules from models and models.wcs, then I'm OK with splitting them, since I won't have to remember where a specific module lives.

@taldcroft

This comment has been minimized.

Show comment Hide comment
@taldcroft

taldcroft Dec 13, 2012

Owner

@nden - sorry, I have been tied up with other things and haven't been able to look at these changes closely. From the comments and commit titles it looks like you made some changes that I'll like. 😃 When things are quieter next week I'll get back to this.

Owner

taldcroft commented Dec 13, 2012

@nden - sorry, I have been tied up with other things and haven't been able to look at these changes closely. From the comments and commit titles it looks like you made some changes that I'll like. 😃 When things are quieter next week I'll get back to this.

@cdeil

This comment has been minimized.

Show comment Hide comment
@cdeil

cdeil Jan 12, 2013

Member

@nden You commented here that you plan to

- Separate optimization methods form fit statistic
- Add parameter confidence intervals

It's not clear to me what the proper home for the fit statistics, optimization methods and confidence interval methods is. E.g. in Sherpa each has it's own sub-package (see here):

  • stats (fit statistics like various Likelihoods and Chi2 functions, see here)
  • optmethods (optimisation methods, see here
  • estmethods (confidence limit estimation methods, e.g. profile likelihood)

I think the fit statistics should go in astropy.stats, actually I'm implementing the Sherpa statistics (and other stats things) at the moment:
https://github.com/astropy/astropy/wiki/What-methods-do-we-want-in-astropy.stats%3F
https://groups.google.com/d/topic/astropy-dev/Zwgafam171E/discussion

Where the optimisers and confidence interval methods (well, at the moment the wrappers around scipy.optimize, not the actual implementations) should live is not clear to me. E.g. the Rolke method I propose to include in astropy.stats is really just a special case of the profile likelihood method. I guess they could go in a new astropy.optimize package or into one of the existing astropy.utils or astropy.stats or astropy.models packages, although in the astropy.models case maybe renaming it to astropy.fitting would be better, because e.g. I only expect to find models in astropy.models. Anyways, this is "just" naming, and I don't have a strong opinion what we should do, I just wanted to mention the question here.

Member

cdeil commented Jan 12, 2013

@nden You commented here that you plan to

- Separate optimization methods form fit statistic
- Add parameter confidence intervals

It's not clear to me what the proper home for the fit statistics, optimization methods and confidence interval methods is. E.g. in Sherpa each has it's own sub-package (see here):

  • stats (fit statistics like various Likelihoods and Chi2 functions, see here)
  • optmethods (optimisation methods, see here
  • estmethods (confidence limit estimation methods, e.g. profile likelihood)

I think the fit statistics should go in astropy.stats, actually I'm implementing the Sherpa statistics (and other stats things) at the moment:
https://github.com/astropy/astropy/wiki/What-methods-do-we-want-in-astropy.stats%3F
https://groups.google.com/d/topic/astropy-dev/Zwgafam171E/discussion

Where the optimisers and confidence interval methods (well, at the moment the wrappers around scipy.optimize, not the actual implementations) should live is not clear to me. E.g. the Rolke method I propose to include in astropy.stats is really just a special case of the profile likelihood method. I guess they could go in a new astropy.optimize package or into one of the existing astropy.utils or astropy.stats or astropy.models packages, although in the astropy.models case maybe renaming it to astropy.fitting would be better, because e.g. I only expect to find models in astropy.models. Anyways, this is "just" naming, and I don't have a strong opinion what we should do, I just wanted to mention the question here.

@keflavich

This comment has been minimized.

Show comment Hide comment
@keflavich

keflavich Feb 21, 2013

Contributor

I'm very interesting in using the astropy modeling tools, so if and when this gets merged, I'll work on a tutorial for it based on blackbody fitting - I'd like to translate this: https://code.google.com/p/agpy/source/browse/trunk/agpy/blackbody.py to astropy, so that it uses astropy's units framework + fitting framework.

But keeping the linked example in mind, I think it would be worthwhile to make "mpfit to astropy.models" and "lmfit-py to astropy.models" guides. I haven't looked at the PR closely yet, but I'd really like to know how it differs from https://github.com/newville/lmfit-py.

Contributor

keflavich commented Feb 21, 2013

I'm very interesting in using the astropy modeling tools, so if and when this gets merged, I'll work on a tutorial for it based on blackbody fitting - I'd like to translate this: https://code.google.com/p/agpy/source/browse/trunk/agpy/blackbody.py to astropy, so that it uses astropy's units framework + fitting framework.

But keeping the linked example in mind, I think it would be worthwhile to make "mpfit to astropy.models" and "lmfit-py to astropy.models" guides. I haven't looked at the PR closely yet, but I'd really like to know how it differs from https://github.com/newville/lmfit-py.

@nden

This comment has been minimized.

Show comment Hide comment
@nden

nden Feb 22, 2013

Contributor

@keflavich It seems this is similar to lmfit-py in many ways. I think the main difference is that we have a formalized Model class and the Parameters (functionally similar to lmfit-py.Parameters) is attached to it but shared between models and fitters. This makes it possible to create composite models. astropy.models supports the same constraints, although after looking at lmfit-py I see that the bounds are calculated in a different way. It probably differs in other details as well. Also, the goal in astropy.models is to create a formal separation of optimization methods and statistics methods so that they can be combined arbitrarily.
I looked briefly at blackbody.py and I think it should be straightforward to convert it to astropy.models.

Contributor

nden commented Feb 22, 2013

@keflavich It seems this is similar to lmfit-py in many ways. I think the main difference is that we have a formalized Model class and the Parameters (functionally similar to lmfit-py.Parameters) is attached to it but shared between models and fitters. This makes it possible to create composite models. astropy.models supports the same constraints, although after looking at lmfit-py I see that the bounds are calculated in a different way. It probably differs in other details as well. Also, the goal in astropy.models is to create a formal separation of optimization methods and statistics methods so that they can be combined arbitrarily.
I looked briefly at blackbody.py and I think it should be straightforward to convert it to astropy.models.

@nden

This comment has been minimized.

Show comment Hide comment
@nden

nden Feb 22, 2013

Contributor

@cdeil Sorry for not responding for such a long time - I was working on other things while waiting for the 0.2 release and this fell through the cracks.

Having separate astropy subpackages for optimization, statistics and estimation methods may make sense in the future. But I think we should try to avoid package proliferation unless necessary. If code functionality is used by several packages it may make sense to separate it. I am not too happy about the name too but I can't come up with a better one. The initial name was "fitting" but at the last coordination meeting it was decided to rename it to "models". I have no strong feeling either way.

Contributor

nden commented Feb 22, 2013

@cdeil Sorry for not responding for such a long time - I was working on other things while waiting for the 0.2 release and this fell through the cracks.

Having separate astropy subpackages for optimization, statistics and estimation methods may make sense in the future. But I think we should try to avoid package proliferation unless necessary. If code functionality is used by several packages it may make sense to separate it. I am not too happy about the name too but I can't come up with a better one. The initial name was "fitting" but at the last coordination meeting it was decided to rename it to "models". I have no strong feeling either way.

@astrofrog

This comment has been minimized.

Show comment Hide comment
@astrofrog

astrofrog Mar 6, 2013

Owner

@taldcroft - can you review this in the near future to see if it addresses your concerns?

I'll also review this within the next week.

Owner

astrofrog commented Mar 6, 2013

@taldcroft - can you review this in the near future to see if it addresses your concerns?

I'll also review this within the next week.

@taldcroft

This comment has been minimized.

Show comment Hide comment
@taldcroft

taldcroft Mar 7, 2013

Owner

@astrofrog - I'll plan to review this within the next week as well.

Owner

taldcroft commented Mar 7, 2013

@astrofrog - I'll plan to review this within the next week as well.

@mdboom

View changes

astropy/models/constraints.py
+ """
+ Parameters
+ ----------
+ fitter: object which supports fitting

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

It's annoying, but there needs to be a space on both sides of the colon in numpydoc parameter lists, otherwise it doesn't see the separation between the name of the argument and the type.

@mdboom

mdboom Mar 8, 2013

Contributor

It's annoying, but there needs to be a space on both sides of the colon in numpydoc parameter lists, otherwise it doesn't see the separation between the name of the argument and the type.

@mdboom

View changes

astropy/models/constraints.py
+
+ def tie_center(model):
+ xcen = 50*model.xsigma
+ return xcen

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

I'm not sure this kind of formatting works here -- we'll have to check. This might have to be moved to an examples section below.

@mdboom

mdboom Mar 8, 2013

Contributor

I'm not sure this kind of formatting works here -- we'll have to check. This might have to be moved to an examples section below.

@mdboom

View changes

astropy/models/constraints.py
+ fitter: object which supports fitting
+ fixed: dict
+ (parameter_name: True/False}
+ parameters to be held fixed during fitting

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

I don't quite understand what fixed is supposed to be from this docstring.

@mdboom

mdboom Mar 8, 2013

Contributor

I don't quite understand what fixed is supposed to be from this docstring.

@mdboom

View changes

astropy/models/constraints.py
+ tied: dict
+ keys are parameter names
+ values are callable/function providing a relationship
+ between parameters. Currently the callable takes a model

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

This needs some punctuation, since Sphinx won't preserve the line endings here. As in:

Keys are parameter names; values are callables/functions providing a 
relationship between parameters.
@mdboom

mdboom Mar 8, 2013

Contributor

This needs some punctuation, since Sphinx won't preserve the line endings here. As in:

Keys are parameter names; values are callables/functions providing a 
relationship between parameters.
@mdboom

View changes

astropy/models/constraints.py
+ ieqcons[j](x0,*args) >= 0.0 in a successfully optimized
+ problem.
+ """
+ self.model = model

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

Should we (for future flexibility) store model in a private member with a public property (like all the other members in this class)?

@mdboom

mdboom Mar 8, 2013

Contributor

Should we (for future flexibility) store model in a private member with a public property (like all the other members in this class)?

This comment has been minimized.

Show comment Hide comment
@nden

nden Mar 13, 2013

Contributor

@mdboom The guide says "Classes should either use direct variable access, or python’s property mechanism ...". Am I wrong in thinking that a property makes sense mostly when something more than just returning or setting the variable needs to be done? The guide for me in this code was to always set model parameters with properties since they are the variables users will interact with. Constraints and fitters keep an instamce of the model as an attribute because they need to manipulate its parameters but users don't need to access constraints.model or fitter.model. I don't see a benefit in making 'model' a property. A private variable - may be but this seems a bit arbitrary. Does this make sense?

@nden

nden Mar 13, 2013

Contributor

@mdboom The guide says "Classes should either use direct variable access, or python’s property mechanism ...". Am I wrong in thinking that a property makes sense mostly when something more than just returning or setting the variable needs to be done? The guide for me in this code was to always set model parameters with properties since they are the variables users will interact with. Constraints and fitters keep an instamce of the model as an attribute because they need to manipulate its parameters but users don't need to access constraints.model or fitter.model. I don't see a benefit in making 'model' a property. A private variable - may be but this seems a bit arbitrary. Does this make sense?

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 13, 2013

Contributor

Ok. What I'm concerned about is user code that gets written that changes
model, and later we want to change how that's implement but are bound by
how we've done it here. I think if users don't need to access it (but
other classes in this package do), it should be a private variable.

On Wed, Mar 13, 2013 at 11:59 AM, Nadia Dencheva
notifications@github.comwrote:

In astropy/models/constraints.py:

  •           return xcen
    
  •        tied ={'xcen':tie_center}
    
  •    bounds: dict
    
  •        keys: parameter names
    
  •        values:  list of length 2 giving the desired range for hte parameter
    
  •    eqcons: list
    
  •        A list of functions of length n such that
    
  •        eqcons[j](x0,*args) == 0.0 in a successfully optimized
    
  •        problem.
    
  •    ineqcons: list
    
  •        A list of functions of length n such that
    
  •        ieqcons[j](x0,*args) >= 0.0 in a successfully optimized
    
  •        problem.
    
  •    """
    
  •    self.model = model
    

@mdboom https://github.com/mdboom The guide says "Classes should either
use direct variable access, or python’s property mechanism ...". Am I wrong
in thinking that a property makes sense mostly when something more than
just returning or setting the variable needs to be done? The guide for me
in this code was to always set model parameters with properties since they
are the variables users will interact with. Constraints and fitters keep an
instamce of the model as an attribute because they need to manipulate its
parameters but users don't need to access constraints.model or
fitter.model. I don't see a benefit in making 'model' a property. A private
variable - may be but this seems a bit arbitrary. Does this make sense?


Reply to this email directly or view it on GitHubhttps://github.com/astropy/astropy/pull/493/files#r3356651
.

Michael Droettboom
http://www.droettboom.com/

@mdboom

mdboom Mar 13, 2013

Contributor

Ok. What I'm concerned about is user code that gets written that changes
model, and later we want to change how that's implement but are bound by
how we've done it here. I think if users don't need to access it (but
other classes in this package do), it should be a private variable.

On Wed, Mar 13, 2013 at 11:59 AM, Nadia Dencheva
notifications@github.comwrote:

In astropy/models/constraints.py:

  •           return xcen
    
  •        tied ={'xcen':tie_center}
    
  •    bounds: dict
    
  •        keys: parameter names
    
  •        values:  list of length 2 giving the desired range for hte parameter
    
  •    eqcons: list
    
  •        A list of functions of length n such that
    
  •        eqcons[j](x0,*args) == 0.0 in a successfully optimized
    
  •        problem.
    
  •    ineqcons: list
    
  •        A list of functions of length n such that
    
  •        ieqcons[j](x0,*args) >= 0.0 in a successfully optimized
    
  •        problem.
    
  •    """
    
  •    self.model = model
    

@mdboom https://github.com/mdboom The guide says "Classes should either
use direct variable access, or python’s property mechanism ...". Am I wrong
in thinking that a property makes sense mostly when something more than
just returning or setting the variable needs to be done? The guide for me
in this code was to always set model parameters with properties since they
are the variables users will interact with. Constraints and fitters keep an
instamce of the model as an attribute because they need to manipulate its
parameters but users don't need to access constraints.model or
fitter.model. I don't see a benefit in making 'model' a property. A private
variable - may be but this seems a bit arbitrary. Does this make sense?


Reply to this email directly or view it on GitHubhttps://github.com/astropy/astropy/pull/493/files#r3356651
.

Michael Droettboom
http://www.droettboom.com/

@mdboom

View changes

astropy/models/fitting.py
+ The purpose of this class is to deal with constraints
+ """
+ def __init__(self, model):
+ __metaclass__ = abc.ABCMeta

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

I think the __metaclass__ declaration needs to be at the top level of the class. i.e. outside of the __init__ function. All this does is assign it to a local variable, if I understand correctly.

@mdboom

mdboom Mar 8, 2013

Contributor

I think the __metaclass__ declaration needs to be at the top level of the class. i.e. outside of the __init__ function. All this does is assign it to a local variable, if I understand correctly.

@mdboom

View changes

astropy/models/fitting.py
+ def __init__(self, model):
+ __metaclass__ = abc.ABCMeta
+
+ self.model = model

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

As above, maybe this should be private with a property?

@mdboom

mdboom Mar 8, 2013

Contributor

As above, maybe this should be private with a property?

+ dealt with in a separate method.
+
+ """
+ raise NotImplementedError

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

raise NotImplementedError()

@mdboom

mdboom Mar 8, 2013

Contributor

raise NotImplementedError()

@mdboom

View changes

astropy/models/fitting.py
+ account for model constraints
+
+ Currently the only fitter that uses a derivative is the
+ NonLinearLSQFitter. This wrapper may neeed to be revised when

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

Put backticks around identifiers so they will be hyperlinked. e.g.

`NonLinearLSQFitter`
@mdboom

mdboom Mar 8, 2013

Contributor

Put backticks around identifiers so they will be hyperlinked. e.g.

`NonLinearLSQFitter`
@mdboom

View changes

astropy/models/fitting.py
+
+ Currently the only fitter that uses a derivative is the
+ NonLinearLSQFitter. This wrapper may neeed to be revised when
+ when other fitters using function derivative are added or when

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

when is repeated here.

@mdboom

mdboom Mar 8, 2013

Contributor

when is repeated here.

@mdboom

View changes

astropy/models/fitting.py
+ try:
+ c = constraintsdef[fname]
+ except KeyError:
+ print("%s does not support fitting with constraints" % fname)

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

Rather than using print, this should probably be a warning, so it can be logged and controlled etc. Also, it seems like the purpose of this is to explain why the KeyError is raised. Maybe it makes sense to just raise a different error with this message in it?

@mdboom

mdboom Mar 8, 2013

Contributor

Rather than using print, this should probably be a warning, so it can be logged and controlled etc. Also, it seems like the purpose of this is to explain why the KeyError is raised. Maybe it makes sense to just raise a different error with this message in it?

This comment has been minimized.

Show comment Hide comment
@nden

nden Mar 13, 2013

Contributor

You are right, this is to explain the KeyError, so I changed it to raise a custom Error.

@nden

nden Mar 13, 2013

Contributor

You are right, this is to explain the KeyError, so I changed it to raise a custom Error.

@mdboom

View changes

astropy/models/constraints.py
+ fmt = ""
+ if any(self._fixed.values()):
+ fixedstr = [par for par in self._fixed if self._fixed[par]]
+ fmt += "fixed=%s, " % str(fixedstr)

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

Our astropy coding guidelines suggest using new-style formatting (str.format) rather than % whenever possible. (There are some weird side cases where we don't because the behavior has changed, but for new code like this we should use format, I think).

@mdboom

mdboom Mar 8, 2013

Contributor

Our astropy coding guidelines suggest using new-style formatting (str.format) rather than % whenever possible. (There are some weird side cases where we don't because the behavior has changed, but for new code like this we should use format, I think).

@mdboom

View changes

astropy/models/fitting.py
+ 'LinearLSQFitter': ['fixed'],
+ }
+
+class ModelLinearityException(Exception):

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

I've been bitten by this myself -- most of the built-in Python exceptions are named SomethingError, not SomethingException, so we should probably follow that convention.

@mdboom

mdboom Mar 8, 2013

Contributor

I've been bitten by this myself -- most of the built-in Python exceptions are named SomethingError, not SomethingException, so we should probably follow that convention.

@mdboom

View changes

astropy/models/fitting.py
+
+ def __call__(self, x, y, z=None, w=None, rcond=None):
+ """
+ Parameters

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

There should be a short description of what this does here. i.e., why would one "call" this instance?

@mdboom

mdboom Mar 8, 2013

Contributor

There should be a short description of what this does here. i.e., why would one "call" this instance?

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

I see the answer is documented in the models.py module docstring -- that's probably ok for the detailed explanation, but maybe all of these just say "Transforms data using the fitter." or something generic like that.

@mdboom

mdboom Mar 8, 2013

Contributor

I see the answer is documented in the models.py module docstring -- that's probably ok for the detailed explanation, but maybe all of these just say "Transforms data using the fitter." or something generic like that.

@mdboom

View changes

astropy/models/fitting.py
+
+ """
+ multiple = False
+ x = np.asarray(x) + 0.0

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

What is the + 0.0 here for? Could it be handled by explicitly setting a dtype in np.asarray?

@mdboom

mdboom Mar 8, 2013

Contributor

What is the + 0.0 here for? Could it be handled by explicitly setting a dtype in np.asarray?

@mdboom

View changes

astropy/models/fitting.py
+
+ if z is None:
+ if x.shape[0] != y.shape[0]:
+ raise ValueError("x and y should have the same length")

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

Do they need to be the same length, or is it good enough that they are broadcastable? Could it be made such?

@mdboom

mdboom Mar 8, 2013

Contributor

Do they need to be the same length, or is it good enough that they are broadcastable? Could it be made such?

This comment has been minimized.

Show comment Hide comment
@nden

nden Mar 13, 2013

Contributor

If x and y are 1D arrays I think it makes sense to require them to be the same size. The intention here is to be able to fit arrays of shapes (n,) and (n,m) where m different sets of y are fit to the same x. But I can see that the error message is not very helpful. May be this sounds better?
"Expected the measured and model data to have the same size"

@nden

nden Mar 13, 2013

Contributor

If x and y are 1D arrays I think it makes sense to require them to be the same size. The intention here is to be able to fit arrays of shapes (n,) and (n,m) where m different sets of y are fit to the same x. But I can see that the error message is not very helpful. May be this sounds better?
"Expected the measured and model data to have the same size"

@mdboom

View changes

astropy/models/fitting.py
+ [setattr(self.model, name, par if par>b[0] else b[0]) for
+ name, par, b in zip(self.model.parnames, fitpars, bounds)]
+ [setattr(self.model, name, par if par<b[1] else b[1]) for
+ name, par, b in zip(self.model.parnames, fitpars, bounds)]

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

This is probably a matter of personal style, but I'm generally not a fan of list comprehensions that don't get assigned to anything. I'd rather just see this as a regular for loop, particularly since both of these are iterating over the same thing.

@mdboom

mdboom Mar 8, 2013

Contributor

This is probably a matter of personal style, but I'm generally not a fan of list comprehensions that don't get assigned to anything. I'd rather just see this as a regular for loop, particularly since both of these are iterating over the same thing.

@mdboom

View changes

astropy/models/models.py
+
+ 'N' - normal (don't do anything to the output)
+ 'T' - transposed
+ 'S' - scalar

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

This should be reformatted in numpydoc format.

@mdboom

mdboom Mar 8, 2013

Contributor

This should be reformatted in numpydoc format.

@mdboom

View changes

astropy/models/models.py
+ "Input parameter '%s' does not "
+ "have the required shape" % name)
+ else:
+ setattr(self, '_'+name, par)

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

This getpar/setpar stuff could be done perhaps more easily with a descriptor class. You basically create a class with __get__ and __set__ members, and then you can assign instances of that class to another class like you would a property. I think that's more straightforward than this plus the lambda/property magic used to define them below.

See this: http://docs.python.org/2/howto/descriptor.html

@mdboom

mdboom Mar 8, 2013

Contributor

This getpar/setpar stuff could be done perhaps more easily with a descriptor class. You basically create a class with __get__ and __set__ members, and then you can assign instances of that class to another class like you would a property. I think that's more straightforward than this plus the lambda/property magic used to define them below.

See this: http://docs.python.org/2/howto/descriptor.html

@mdboom

View changes

astropy/models/models.py
+
+ Returns
+ -------
+ an instance of CompositeModel

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

The Returns section needs to be in the same parameter format, i.e.:

model : CompositeModel
@mdboom

mdboom Mar 8, 2013

Contributor

The Returns section needs to be in the same parameter format, i.e.:

model : CompositeModel
@mdboom

View changes

astropy/models/models.py
+ Represents a general polynomial of degree n:
+
+ P(x,y) = c0_0 + c1_0*x + ...+ cn_0*x**n + c0_1*y + ...+ c0_n*y**n +
+ c1_1*x*y + c1_2*x*y**2 + ... + c1_(n-1)*x*y**(n-1)+ ... + c(n-1)_1*x**(n-1)*y

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

This would be nice as a math directive so it gets rendered nicely as LaTeX in the docs.

@mdboom

mdboom Mar 8, 2013

Contributor

This would be nice as a math directive so it gets rendered nicely as LaTeX in the docs.

@mdboom

View changes

astropy/models/parameters.py
+ paramdim: int
+ parameter dimension
+ """
+ self.paramdim = paramdim

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

private member with property?

@mdboom

mdboom Mar 8, 2013

Contributor

private member with property?

+ 'Pix2Sky_CEA', 'Sky2Pix_CEA', 'Pix2Sky_COP', 'Sky2Pix_COP',
+ 'Pix2Sky_CYP', 'Sky2Pix_CYP', 'Pix2Sky_MER', 'Sky2Pix_MER',
+ 'Pix2Sky_SIN', 'Sky2Pix_SIN', 'Pix2Sky_STG', 'Sky2Pix_STG',
+ 'Pix2Sky_TAN', 'Sky2Pix_TAN']

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

I understand these three-letter projection codes come from FITS WCS and are probably familiar to many, but I for one always have trouble remembering them. Maybe we should have aliases with longer names, e.g.:

Pix2Sky_Tangent = Pix2Sky_TAN
@mdboom

mdboom Mar 8, 2013

Contributor

I understand these three-letter projection codes come from FITS WCS and are probably familiar to many, but I for one always have trouble remembering them. Maybe we should have aliases with longer names, e.g.:

Pix2Sky_Tangent = Pix2Sky_TAN

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

Also -- question as to whether we want to use the word sky or world here. I know wcslib uses sky, but I think @astrofrog at one point explained that a better word would be world, but I leave that up for others to decide.

@mdboom

mdboom Mar 8, 2013

Contributor

Also -- question as to whether we want to use the word sky or world here. I know wcslib uses sky, but I think @astrofrog at one point explained that a better word would be world, but I leave that up for others to decide.

This comment has been minimized.

Show comment Hide comment
@nden

nden Mar 10, 2013

Contributor

I agree in many cases it's appropriate to have 'world' instead of 'sky'. In this case though the world coordinates are really sky coordinates. But I don't have strong feelings about this and would also leave it to other people to decide.

@nden

nden Mar 10, 2013

Contributor

I agree in many cases it's appropriate to have 'world' instead of 'sky'. In this case though the world coordinates are really sky coordinates. But I don't have strong feelings about this and would also leave it to other people to decide.

This comment has been minimized.

Show comment Hide comment
@wkerzendorf

wkerzendorf Mar 18, 2013

Member

I prefer world as in an n-dimensional theory simulation where axes map to e.g. density, temperature and hydrogen abundance - sky would not make a lot of sense.

@wkerzendorf

wkerzendorf Mar 18, 2013

Member

I prefer world as in an n-dimensional theory simulation where axes map to e.g. density, temperature and hydrogen abundance - sky would not make a lot of sense.

This comment has been minimized.

Show comment Hide comment
@nden

nden Mar 19, 2013

Contributor

In my mind 'world' in the context of WCS means a physical quantity. The classes in projections.py represent geometrical relations. It just happens that 'sphere' in astronomy is 'sky'. We could abstract the names even more and name them something like 'pix_to_sphere' or 'plain_to_sphere' but certainly not 'world'. Unless I'm mistaken...

@nden

nden Mar 19, 2013

Contributor

In my mind 'world' in the context of WCS means a physical quantity. The classes in projections.py represent geometrical relations. It just happens that 'sphere' in astronomy is 'sky'. We could abstract the names even more and name them something like 'pix_to_sphere' or 'plain_to_sphere' but certainly not 'world'. Unless I'm mistaken...

@@ -0,0 +1,36 @@
+# Thu 16:08:18 31-Mar-2011

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

This file has a fits extension, but doesn't look like a FITS file. Is that deliberate?

@mdboom

mdboom Mar 8, 2013

Contributor

This file has a fits extension, but doesn't look like a FITS file. Is that deliberate?

This comment has been minimized.

Show comment Hide comment
@nden

nden Mar 10, 2013

Contributor

It's the output of IRAF's identify task. The naming convention for their database file is to prepend 'id' to the fits file name. It doesn't really matter what the file name is for this tests since I just parse the IRAF database file to compare the fit results. I can change it if it's too confusing.

@nden

nden Mar 10, 2013

Contributor

It's the output of IRAF's identify task. The naming convention for their database file is to prepend 'id' to the fits file name. It doesn't really matter what the file name is for this tests since I just parse the IRAF database file to compare the fit results. I can change it if it's too confusing.

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 11, 2013

Contributor

Ok -- it's probably fine in that case if that's the convention. @eteq: As the "data" guy, what do you think?

@mdboom

mdboom Mar 11, 2013

Contributor

Ok -- it's probably fine in that case if that's the convention. @eteq: As the "data" guy, what do you think?

@mdboom

View changes

astropy/models/tests/testfitters.py
+from numpy import linalg
+from numpy.testing import utils
+from scipy import optimize
+from data import dpath

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Mar 8, 2013

Contributor

To load data files, astropy has utils.data.get_pkg_data_filename and friends. I have nothing against with this approach here, but maybe we should use utils.data to be consistent with other packages?

@mdboom

mdboom Mar 8, 2013

Contributor

To load data files, astropy has utils.data.get_pkg_data_filename and friends. I have nothing against with this approach here, but maybe we should use utils.data to be consistent with other packages?

nden and others added some commits Apr 2, 2013

Fix some issues (mostly cleanup) in models
There are things that were pointed out by Eclipse PyDev code analysis
Rename astropy.util to astropy.utils
This is more consistent with other packages I believe.
Rename test_pars.py to test_parameters.py
To be consistent with the module name parameters.py.
Fix a serious bug where the x,y positional arguments were flipped whe…
…n an analytical derivative was passed to a non-linear fitting algorithm. Add tests.
@nden

This comment has been minimized.

Show comment Hide comment
@nden

nden Apr 22, 2013

Contributor

What is the next step in this painful process of getting the PR merged?

The documentation build on Travis fails because of 2 warnings. Is this what's holding off the merge? If so, I've already asked but will ask again, if someone familiar with the sphinx extensions, can tell where the warnings are coming from.

None:1: WARNING: citation not found: R7
None:1: WARNING: citation not found: R8

Contributor

nden commented Apr 22, 2013

What is the next step in this painful process of getting the PR merged?

The documentation build on Travis fails because of 2 warnings. Is this what's holding off the merge? If so, I've already asked but will ask again, if someone familiar with the sphinx extensions, can tell where the warnings are coming from.

None:1: WARNING: citation not found: R7
None:1: WARNING: citation not found: R8

@eteq

This comment has been minimized.

Show comment Hide comment
@eteq

eteq Apr 22, 2013

Owner

Ooh, this was a tricky one @nden - the problem was that in a couple places you had references in your docstrings that were used in the first sentence of the docstring. Because automodapi uses the first sentence to populate the function/class table, it was pulling those references into the table... but the references in question weren't in the page the table is on, so it couldn't find a citation.

Anyway, I think I got it to work by just moving the references to a new sentence (e.g., eteq/astropy@models#L1R976) - I've got them running in travis now on my branch, and assuming they pass, I will go ahead and merge this.

So hopefully no more steps!

Owner

eteq commented Apr 22, 2013

Ooh, this was a tricky one @nden - the problem was that in a couple places you had references in your docstrings that were used in the first sentence of the docstring. Because automodapi uses the first sentence to populate the function/class table, it was pulling those references into the table... but the references in question weren't in the page the table is on, so it couldn't find a citation.

Anyway, I think I got it to work by just moving the references to a new sentence (e.g., eteq/astropy@models#L1R976) - I've got them running in travis now on my branch, and assuming they pass, I will go ahead and merge this.

So hopefully no more steps!

@eteq eteq merged commit 437a969 into astropy:master Apr 23, 2013

1 check failed

default The Travis build failed
Details
@eteq

This comment has been minimized.

Show comment Hide comment
@eteq

eteq Apr 23, 2013

Owner

Merged! Thanks for all your effort on this @nden! Sorry again that it has taken so long.

Owner

eteq commented Apr 23, 2013

Merged! Thanks for all your effort on this @nden! Sorry again that it has taken so long.

@wkerzendorf

This comment has been minimized.

Show comment Hide comment
@wkerzendorf

wkerzendorf Apr 23, 2013

Member

I'm looking forward to playing around with this! Good Job!

Member

wkerzendorf commented Apr 23, 2013

I'm looking forward to playing around with this! Good Job!

@nden

This comment has been minimized.

Show comment Hide comment
@nden

nden Apr 23, 2013

Contributor

@eteq Thanks for figuring out the problem with the references, I was lost.
I'm looking forward to comments about its functionality as others try it out.

Contributor

nden commented Apr 23, 2013

@eteq Thanks for figuring out the problem with the references, I was lost.
I'm looking forward to comments about its functionality as others try it out.

@astrofrog

This comment has been minimized.

Show comment Hide comment
@astrofrog

astrofrog Apr 23, 2013

Owner

@nden - thank you for all your work on this, this will be a great addition to Astropy!

I'm thinking maybe this should be advertised on astropy-dev now so that people who keep up with the latest dev version can play with it and report any issues?

Owner

astrofrog commented Apr 23, 2013

@nden - thank you for all your work on this, this will be a great addition to Astropy!

I'm thinking maybe this should be advertised on astropy-dev now so that people who keep up with the latest dev version can play with it and report any issues?

@nden

This comment has been minimized.

Show comment Hide comment
@nden

nden Apr 23, 2013

Contributor

@astrofrog Good idea! It's best if this is tested as much as possible before it is released. Would you like to send a note or do you want me to do it?

Contributor

nden commented Apr 23, 2013

@astrofrog Good idea! It's best if this is tested as much as possible before it is released. Would you like to send a note or do you want me to do it?

@astrofrog

This comment has been minimized.

Show comment Hide comment
@astrofrog

astrofrog Apr 23, 2013

Owner

Cool - I can send a note to the list!

Owner

astrofrog commented Apr 23, 2013

Cool - I can send a note to the list!

@pllim

This comment has been minimized.

Show comment Hide comment
@pllim

pllim Oct 15, 2013

Member

@nden - Should this be __call__ instead of call?

@nden - Should this be __call__ instead of call?

This comment has been minimized.

Show comment Hide comment
@nden

nden Oct 16, 2013

Contributor

@plim and yes

Contributor

nden replied Oct 16, 2013

@plim and yes

@pllim

This comment has been minimized.

Show comment Hide comment
@pllim

pllim Oct 15, 2013

Member

@nden - Should this be __init__ instead of init? Also, indentation seems off for the class methods.

@nden - Should this be __init__ instead of init? Also, indentation seems off for the class methods.

This comment has been minimized.

Show comment Hide comment
@nden

nden Oct 16, 2013

Contributor

@plim yes

Contributor

nden replied Oct 16, 2013

@plim yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment