Name the module motion_models, and alias it where the argument shadows it

The module keeps the underscore, as preferred. What cannot move is the other
side of the collision: every `motion_models` binding in the package is either
the public keyword argument of MosaicSelfRef, MosaicToRef, fit_motion_models,
infer_positions, determine_motion_models and organize_motion_models, or that
same parameter reassigned inside its own function. Renaming it would change the
documented API that the examples, the notebook and outside scripts call, which
is a much larger break than a module name.

So the two library files that use both -- align.py and startables.py -- import
the module as `mmodels`, with a comment at the import saying why, and
test_align.py does the same for the local of that name in several of its tests.
Everywhere the name is free (transforms.py, the other tests, the docs, the
notebook, the benchmark script) it is imported and referred to plainly as
motion_models.

Verified beyond the suite that the shadowed call path still works from outside:
fit_motion_models(motion_models=['Linear']) on a table fits Linear, and
organize_motion_models('linear') resolves, with flystar.motion_models importable
under its own name. Full suite passes, docs build clean, and the API reference is
back at api/flystar/motion_models/.

Author: wei-lingfeng

docs/alignment.rst

@@ -244,7 +244,7 @@ These are handed straight to
        epochs to support, unless a ``motion_model_input`` column requests
        otherwise. Add ``'Linear'`` if you want proper motions at all -- the
        default fits none. Accepts a single name, a list of names, the
-       :class:`~flystar.motionmodels.MotionModel` subclasses themselves, or a
+       :class:`~flystar.motion_models.MotionModel` subclasses themselves, or a
        mixture; see the note below.
    * - ``fixed_params_dict``
      - ``None``
@@ -268,8 +268,8 @@ These are handed straight to
       motion_models='Linear'                      # a single name
       motion_models=['Linear']                    # a list of names
       motion_models=['Linear', 'Acceleration']    # several
-      motion_models=[motionmodels.Linear]         # the classes themselves
-      motion_models=[motionmodels.Linear, 'Acceleration']   # mixed
+      motion_models=[motion_models.Linear]         # the classes themselves
+      motion_models=[motion_models.Linear, 'Acceleration']   # mixed
       motion_models='linear'                      # case does not matter
 
    The valid names are ``'Empty'``, ``'Fixed'``, ``'Linear'``,

docs/benchmark_motion_models.py

@@ -74,7 +74,7 @@ def make_table(n_stars, n_epochs, seed=1):
 
 def time_one(branch, model, n_epochs, n_stars=N_STARS):
     """Time a single fit, and report which model the stars actually got."""
-    from flystar import motionmodels as MM
+    from flystar import motion_models as MM
 
     tab = make_table(n_stars, n_epochs)
 

docs/conf.py

@@ -86,7 +86,7 @@
 #
 # autoapi walks the source statically, so every module on this branch is picked
 # up automatically -- including ones that post-date the old hand-maintained
-# automodapi list (motionmodels, parallax, startables ...).
+# automodapi list (motion_models, parallax, startables ...).
 
 autoapi_dirs = ['../flystar']
 autoapi_root = 'api'

docs/examples/motion_model_example.ipynb

@@ -100,9 +100,9 @@
    "source": [
     "import numpy as np\n",
     "import matplotlib.pyplot as plt\n",
-    "from flystar import motionmodels\n",
+    "from flystar import motion_models\n",
     "from flystar.startables import StarTable\n",
-    "from flystar.motionmodels import Empty, Fixed, Linear, Acceleration, Parallax"
+    "from flystar.motion_models import Empty, Fixed, Linear, Acceleration, Parallax"
    ]
   },
   {
@@ -876,7 +876,7 @@
      "name": "stderr",
      "output_type": "stream",
      "text": [
-      "/Users/lwei-local/Software/flystar/flystar/motionmodels.py:346: OptimizeWarning: Degree of freedom <= 0 for some star(s). Covariance of the parameters could not be estimated. Setting parameter uncertainties to np.inf.\n",
+      "/Users/lwei-local/Software/flystar/flystar/motion_models.py:346: OptimizeWarning: Degree of freedom <= 0 for some star(s). Covariance of the parameters could not be estimated. Setting parameter uncertainties to np.inf.\n",
       "  result = self.run_fit(\n"
      ]
     }
@@ -1167,7 +1167,7 @@
     }
    ],
    "source": [
-    "all_mm_map = motionmodels.motion_model_map()\n",
+    "all_mm_map = motion_models.motion_model_map()\n",
     "tab['n_required'] = np.array([all_mm_map[mm].n_params for mm in tab['motion_model_input']], dtype=int)\n",
     "tab[['n_fit', 'n_required', 'motion_model_input', 'motion_model_used']]"
    ]

docs/motion_models.rst

@@ -5,8 +5,8 @@ Motion models
 Once every epoch sits in a common frame, each star has a time series of
 positions, and the question becomes what curve to put through it. A *motion
 model* is that curve. On this branch these are first-class pluggable classes in
-:mod:`flystar.motionmodels`, all deriving from
-:class:`~flystar.motionmodels.MotionModel`, and FlyStar chooses between them
+:mod:`flystar.motion_models`, all deriving from
+:class:`~flystar.motion_models.MotionModel`, and FlyStar chooses between them
 **per star** rather than imposing one on the whole table.
 
 This is the part of the API that differs most from ``main``.
@@ -32,27 +32,27 @@ Throughout, :math:`\Delta t \equiv t - t_0`.
      - ``n_params``
      - Fit parameters
      - Position model
-   * - :class:`~flystar.motionmodels.Empty`
+   * - :class:`~flystar.motion_models.Empty`
      - 0
      - --
      - :math:`x(t) =` ``fill_value`` (NaN by default), :math:`\sigma_x(t) = \infty`.
        For stars with no usable detection at all.
-   * - :class:`~flystar.motionmodels.Fixed`
+   * - :class:`~flystar.motion_models.Fixed`
      - 1
      - :math:`x_0,\ y_0`
      - :math:`x(t) = x_0`, the weighted mean position -- no time dependence.
        For stars seen once, or held still on purpose.
-   * - :class:`~flystar.motionmodels.Linear`
+   * - :class:`~flystar.motion_models.Linear`
      - 2
      - :math:`x_0, v_x,\ y_0, v_y`
      - :math:`x(t) = x_0 + v_x\,\Delta t`. Constant proper motion; the
        workhorse.
-   * - :class:`~flystar.motionmodels.Acceleration`
+   * - :class:`~flystar.motion_models.Acceleration`
      - 3
      - :math:`x_0, v_{x0}, a_x,\ y_0, v_{y0}, a_y`
      - :math:`x(t) = x_0 + v_{x0}\,\Delta t + \tfrac{1}{2} a_x\,\Delta t^2`.
        For stars whose motion visibly curves.
-   * - :class:`~flystar.motionmodels.Parallax`
+   * - :class:`~flystar.motion_models.Parallax`
      - 3
      - :math:`x_0, v_x, \pi,\ y_0, v_y`
      - :math:`x(t) = x_0 + v_x\,\Delta t + \pi\,P_x(t)`. Linear motion plus
@@ -68,7 +68,7 @@ coordinates,
    y(t) &= y_0 + v_y\,(t - t_0) + \pi\,P_y(t)
 
 :math:`\boldsymbol{P}(t)` is computed by
-:meth:`~flystar.motionmodels.Parallax.calc_parallax_vector` from the star's
+:meth:`~flystar.motion_models.Parallax.calc_parallax_vector` from the star's
 ``ra``/``dec``, the position angle ``pa``, and the observatory location
 ``obsLocation``. Note that ``fit_motion_models`` currently applies a single
 ``obsLocation`` to every star in the table.
@@ -225,7 +225,7 @@ from an earlier, more complex fit.
 Choosing a model versus propagating with one
 --------------------------------------------
 
-:func:`~flystar.motionmodels.determine_motion_models` answers a related but
+:func:`~flystar.motion_models.determine_motion_models` answers a related but
 distinct question, and the ``motion_models`` argument separates them:
 
 * Pass your list to ask *which of the models I requested was this star fit
@@ -238,7 +238,7 @@ distinct question, and the ``motion_models`` argument separates them:
 The time-argument contract
 ==========================
 
-Every model's :meth:`~flystar.motionmodels.MotionModel.model`, and
+Every model's :meth:`~flystar.motion_models.MotionModel.model`, and
 :meth:`~flystar.startables.StarTable.infer_positions`, take times under one
 rule: **shape decides meaning.** Nothing is inferred from ``len(t)`` happening
 to equal ``N_stars``.
@@ -262,7 +262,7 @@ to equal ``N_stars``.
 For **one time per star**, pass a column vector ``t[:, np.newaxis]`` of shape
 ``(N_stars, 1)``. A bare 1D array of length ``N_stars`` means a shared grid, not
 per-star times. Any other shape raises ``ValueError`` rather than being guessed
-at. See :func:`~flystar.motionmodels.broadcast_times`.
+at. See :func:`~flystar.motion_models.broadcast_times`.
 
 Propagating the whole table to a single new epoch does not need the
 column-vector form -- pass the scalar epoch and let each star's own :math:`t_0`
@@ -285,8 +285,8 @@ Weighting
 ---------
 
 ``weighting`` decides how a per-epoch uncertainty becomes a fit weight, via
-:func:`~flystar.motionmodels.sigma_from_error` and then
-:func:`~flystar.motionmodels.weight_from_sigma`, which computes
+:func:`~flystar.motion_models.sigma_from_error` and then
+:func:`~flystar.motion_models.weight_from_sigma`, which computes
 :math:`w = 1/\sigma^2`:
 
 .. list-table::
@@ -352,7 +352,7 @@ Unusable uncertainties get weight zero, not a bad weight
 
 A naive :math:`1/\sigma^2` turns a missing or pathological uncertainty into an
 infinite or NaN weight, corrupting the whole sum rather than excluding one
-point. :func:`~flystar.motionmodels.weight_from_sigma` instead assigns
+point. :func:`~flystar.motion_models.weight_from_sigma` instead assigns
 **exactly zero** whenever :math:`\sigma` is NaN, infinite, exactly zero, or so
 small that squaring it underflows -- and to any epoch marked invalid. Such an
 epoch drops cleanly out of both the fit and the :math:`\chi^2`.
@@ -396,7 +396,7 @@ per-star fit cannot see.
 Fitting: one star or a whole table
 ==================================
 
-:meth:`~flystar.motionmodels.MotionModel.fit` handles both, dispatching on
+:meth:`~flystar.motion_models.MotionModel.fit` handles both, dispatching on
 dimensionality:
 
 * **1D** arrays of shape ``(n_epochs,)`` -- a single star, already filtered to
@@ -760,10 +760,10 @@ Adding new models
 =================
 
 New motion models are welcome, and adding one does not mean touching the
-fitting machinery. A model is a single class in :mod:`flystar.motionmodels`
-that subclasses :class:`~flystar.motionmodels.MotionModel`, declares what it
+fitting machinery. A model is a single class in :mod:`flystar.motion_models`
+that subclasses :class:`~flystar.motion_models.MotionModel`, declares what it
 fits and what it needs held fixed, and implements two methods. There is no
-registry to edit: :func:`~flystar.motionmodels.motion_model_map` discovers
+registry to edit: :func:`~flystar.motion_models.motion_model_map` discovers
 models through ``MotionModel.__subclasses__()``, so the class is selectable by
 name -- ``motion_models=['Wobble']`` -- as soon as the module defining it is
 imported.
@@ -825,7 +825,7 @@ What to implement
      - ``(t, fit_params, fit_param_errs=None, fixed_params_dict=None)``,
        returning ``(x, y)`` -- or ``(x, y, xe, ye)`` when errors are passed in.
        This is what propagation calls, so it must honour the time-argument
-       contract above; use :func:`~flystar.motionmodels.broadcast_times`
+       contract above; use :func:`~flystar.motion_models.broadcast_times`
        rather than reimplementing it.
    * - ``run_fit``
      - ``(t, x, y, xe, ye, valid, fixed_params_dict=None, weighting='var',

docs/overview.rst

@@ -80,7 +80,7 @@ fraction of a year. Nothing else is expected of you.
 
 Where a uniform timescale is genuinely required, FlyStar converts internally.
 The parallax model needs the Earth's barycentric position, whose ephemeris is
-indexed in TDB, so :class:`~flystar.motionmodels.Parallax` does the
+indexed in TDB, so :class:`~flystar.motion_models.Parallax` does the
 ``utc -> tdb`` conversion itself before evaluating
 :func:`~flystar.parallax.parallax_in_direction`.
 
@@ -136,7 +136,7 @@ Where the pieces live
    * - :mod:`flystar.transforms`
      - Coordinate transformation models -- shifts, four-parameter,
        polynomial, Legendre, and spline/clipped variants.
-   * - :mod:`flystar.motionmodels`
+   * - :mod:`flystar.motion_models`
      - Per-star motion models and the machinery that chooses between them.
    * - :mod:`flystar.align`
      - The iterative match/transform/average loop that drives everything else.

flystar/align.py

@@ -8,7 +8,11 @@
 import numpy as np
 import matplotlib.pyplot as plt
 from tqdm import tqdm
-from flystar import match, transforms, plots, motionmodels
+from flystar import match, transforms, plots
+# Aliased: 'motion_models' is the public keyword argument of the aligners
+# and fit_motion_models, and the local it becomes, so importing the module
+# under its own name would shadow it inside the functions that need it.
+from flystar import motion_models as mmodels
 from flystar.starlists import StarList
 from flystar.startables import StarTable
 from astropy.table import Table, Column, vstack
@@ -376,7 +380,7 @@ def __init__(
         self.match_sigma_mag = match_sigma_mag
 
         # Organize motion models into a list of MotionModel classes, sorted by increasing number of parameters.
-        self.motion_models = motionmodels.organize_motion_models(motion_models)
+        self.motion_models = mmodels.organize_motion_models(motion_models)
 
         # if motion_model_for_new_star is None:
         #     self.motion_model_for_new_star = self.motion_models[-1]
@@ -1147,7 +1151,7 @@ def setup_ref_table_from_starlist(self, star_list):
         """
         col_arrays = {}
 
-        motion_model_col_names = motionmodels.all_motion_model_param_names(with_errors=True, with_fixed=True) + ['m0','m0_err','use_in_trans', 'motion_model_input', 'motion_model_used']
+        motion_model_col_names = mmodels.all_motion_model_param_names(with_errors=True, with_fixed=True) + ['m0','m0_err','use_in_trans', 'motion_model_input', 'motion_model_used']
         for col_name in star_list.colnames:
             if col_name == 'name':
                 # The "name" column will be 1D; but we will also add a "name_in_list" column.
@@ -1513,7 +1517,7 @@ def update_ref_table_aggregates(self, keep_orig=None, n_boot=0, seed=None, proce
                 motion_model_class_names += self.ref_table['motion_model_used'][keep_orig].tolist()
                 vals_orig['motion_model_used'] = self.ref_table['motion_model_used'][keep_orig]
                 vals_orig['n_params'] = self.ref_table['n_params'][keep_orig]
-            motion_model_col_names = motionmodels.motion_model_param_names(motion_model_class_names, with_errors=True, with_fixed=True)
+            motion_model_col_names = mmodels.motion_model_param_names(motion_model_class_names, with_errors=True, with_fixed=True)
             for mm in motion_model_col_names:
                 if mm in self.ref_table.keys():
                     vals_orig[mm] = self.ref_table[mm][keep_orig]
@@ -1939,15 +1943,15 @@ def calc_bootstrap_errors(self, n_boot=100, seed=None, boot_epochs_min=-1, calc_
         if 'Fixed' not in motion_model_list:
             motion_model_list.append('Fixed')
 
-        motion_col_list = motionmodels.motion_model_param_names(motion_model_list, with_errors=False, with_fixed=False)
+        motion_col_list = mmodels.motion_model_param_names(motion_model_list, with_errors=False, with_fixed=False)
         if calc_vel_in_bootstrap:
             motion_boot_sum = {}
             motion2_boot_sum = {}
             for col in motion_col_list:
                 motion_boot_sum[col] = np.zeros((len(ref_table['x'])))
                 motion2_boot_sum[col] = np.zeros((len(ref_table['x'])))
 
-        all_mm_map = motionmodels.motion_model_map()
+        all_mm_map = mmodels.motion_model_map()
         motion_model_list = [all_mm_map[mm_name] for mm_name in motion_model_list]
         motion_boot_min_epochs = np.max([mm.n_params for mm in motion_model_list])
 
@@ -2616,7 +2620,7 @@ def = None. If not None, then this should contain an array or list of transform
 
         # If motion_model_used in columns but params columns are missing, raise a warning and remove motion_model_used column to avoid confusion.
         # if 'motion_model_used' in self.ref_list.colnames:
-        #     motion_model_params = motionmodels.motion_model_param_names(np.unique(self.ref_list['motion_model_used']), with_errors=False, with_fixed=True)
+        #     motion_model_params = mmodels.motion_model_param_names(np.unique(self.ref_list['motion_model_used']), with_errors=False, with_fixed=True)
         #     missing_params = [param for param in motion_model_params if (param not in self.ref_list.colnames) and (f'{param}_err' not in self.ref_list.colnames) and (param not in self.fixed_params_dict.keys())]
         #     if len(missing_params) > 0:
         #         warnings.warn("Warning: 'motion_model_used' column found in ref_list, but the following motion model parameter columns are missing: " + ", ".join(missing_params) + ". Removing 'motion_model_used' column to avoid confusion.")
@@ -2995,8 +2999,8 @@ def infer_positions(t, startable, motion_models=None, fixed_params_dict=None, re
     if motion_models is None:
         # Setting the default to None to avoid mutable default argument issue
         # See https://stackoverflow.com/questions/15189245/assigning-class-variable-as-default-value-to-class-method-argument
-        motion_models = [motionmodels.Empty, motionmodels.Fixed]
-    all_mm_map = motionmodels.motion_model_map()
+        motion_models = [mmodels.Empty, mmodels.Fixed]
+    all_mm_map = mmodels.motion_model_map()
     if all(isinstance(mm, str) for mm in motion_models):
         mm_names = motion_models
         motion_models = [all_mm_map[mm] for mm in motion_models]
@@ -3005,9 +3009,9 @@ def infer_positions(t, startable, motion_models=None, fixed_params_dict=None, re
 
     # Always add Empty and Fixed in motion models
     if 'Fixed' not in mm_names:
-        motion_models.insert(0, motionmodels.Fixed)
+        motion_models.insert(0, mmodels.Fixed)
     if 'Empty' not in mm_names:
-        motion_models.insert(0, motionmodels.Empty)
+        motion_models.insert(0, mmodels.Empty)
 
     # Otherwise, infer positions using the most complex motion model with the existing columns, until it reaches Fixed or Empty
     # Sort motion models inversely by mm.n_params
@@ -3039,13 +3043,13 @@ def determine_motion_models(startable, motion_models=None, fixed_params_dict=Non
     """Determine, per star, which motion model to use.
 
     Thin wrapper kept for backward compatibility -- the implementation lives in
-    motionmodels.determine_motion_models, so that startables (which align
+    mmodels.determine_motion_models, so that startables (which align
     imports) can use it too. See there for the precedence rules.
 
     The `processes`/`chunksize` arguments are unused; the implementation is
     vectorized. They are accepted so existing calls keep working.
     """
-    return motionmodels.determine_motion_models(
+    return mmodels.determine_motion_models(
         startable, motion_models=motion_models, fixed_params_dict=fixed_params_dict
     )
 
@@ -3082,7 +3086,7 @@ def setup_ref_table_from_starlist(star_list, motion_models):
     array in the original reference star list.
     """
     col_arrays = {}
-    motion_model_col_names = motionmodels.motion_model_param_names(motion_models, with_errors=True)
+    motion_model_col_names = mmodels.motion_model_param_names(motion_models, with_errors=True)
     for col_name in star_list.colnames:
         if col_name == 'name':
             # The "name" column will be 1D; but we will also add a "name_in_list" column.
@@ -3276,7 +3280,7 @@ def add_rows_for_new_stars(ref_table, star_list, idx_list, motion_model_name='Fi
     idx_lis_new = np.array(list(set(idx_lis_orig) - set(idx_list)))
     N_newstars = len(idx_lis_new)
 
-    mm_map = motionmodels.motion_model_map()
+    mm_map = mmodels.motion_model_map()
     mm = mm_map[motion_model_name]
 
     # Add optional fixed params default values into fixed params dict, prioritizing values in fixed_params_dict
@@ -3948,7 +3952,7 @@ def transform_from_object(starlist, transform):
     # For more complicated motion_models,
     #  we can't easily transform them, set the values to nans and refit later.
     if mot:
-        motion_model_params = motionmodels.motion_model_param_names()
+        motion_model_params = mmodels.motion_model_param_names()
         for param in motion_model_params:
             if param in keys:
                 starlist_f[param] = np.nan