use class-structure for built-in features

kecnry · kecnry · commit 8cb812a87604 · 2025-08-12T08:44:47.000-04:00
diff --git a/phoebe/features/common.py b/phoebe/features/common.py
@@ -5,6 +5,10 @@ class BaseFeature:
     def __init__(self, **kwargs):
         self.kwargs = kwargs
 
+    @classmethod
+    def create_feature_parameters(self, feature, **kwargs):
+        raise NotImplementedError("create_feature_parameters must be implemented in the feature subclass")
+
     @classmethod
     def parse_from_feature_ps(cls, b, feature_ps, param_list):
         _skip_filter_checks = {'check_default': False,
@@ -28,8 +32,3 @@ def parse_bundle(cls, b, feature_ps):
     @classmethod
     def from_bundle(cls, b, feature_ps):
         return cls(**cls.parse_bundle(b, feature_ps))
-
-    @classmethod
-    def get_parameters(self, **kwargs):
-        raise NotImplementedError("get_parameters must be implemented in the feature subclass")
-
diff --git a/phoebe/features/component_features.py b/phoebe/features/component_features.py
@@ -2,6 +2,7 @@
 import astropy.units as u
 
 import phoebe.parameters.feature as _parameters_feature
+from phoebe.parameters import FloatParameter, ParameterSet, constraint
 from phoebe.features.common import BaseFeature
 
 import logging
@@ -115,6 +116,46 @@ class Spot(ComponentFeature):
     remeshing_required = False
     proto_coords = True
 
+    @classmethod
+    def create_feature_parameters(cls, feature, **kwargs):
+        """
+        Create a <phoebe.parameters.ParameterSet> for a spot feature.
+
+        Generally, this will be used as an input to the kind argument in
+        <phoebe.frontend.bundle.Bundle.add_feature>.  If attaching through
+        <phoebe.frontend.bundle.Bundle.add_feature>, all `**kwargs` will be
+        passed on to set the values as described in the arguments below.  Alternatively,
+        see <phoebe.parameters.ParameterSet.set_value> to set/change the values
+        after creating the Parameters.
+
+        Allowed to attach to:
+        * components with kind: star
+        * datasets: not allowed
+
+        Arguments
+        ----------
+        * `colat` (float/quantity, optional): colatitude of the center of the spot
+            wrt spin axis.
+        * `long` (float/quantity, optional): longitude of the center of the spot wrt
+            spin axis.
+        * `radius` (float/quantity, optional): angular radius of the spot.
+        * `relteff` (float/quantity, optional): temperature of the spot relative
+            to the intrinsic temperature.
+
+        Returns
+        --------
+        * (<phoebe.parameters.ParameterSet>, list): ParameterSet of all newly created
+            <phoebe.parameters.Parameter> objects and a list of all necessary
+            constraints.
+        """
+        params = []
+        params += [FloatParameter(qualifier="colat", value=kwargs.get('colat', 0.0), default_unit=u.deg, description='Colatitude of the center of the spot wrt spin axis')]
+        params += [FloatParameter(qualifier="long", value=kwargs.get('long', 0.0), default_unit=u.deg, description='Longitude of the center of the spot wrt spin axis')]
+        params += [FloatParameter(qualifier='radius', value=kwargs.get('radius', 1.0), default_unit=u.deg, description='Angular radius of the spot')]
+        params += [FloatParameter(qualifier='relteff', value=kwargs.get('relteff', 1.0), limits=(0.,None), default_unit=u.dimensionless_unscaled, description='Temperature of the spot relative to the intrinsic temperature')]
+
+        return ParameterSet(params), []
+
     @classmethod
     def parse_bundle(cls, b, feature_ps):
         """
@@ -188,9 +229,9 @@ def pointing_vector(self, s, time):
         exp = np.cross(eyp, ezp)
 
         # now we can express the pointing vector in terms of the primed basis
-        pv = (np.sin(self._colat)*np.cos(longitude)*exp +
-              np.sin(self._colat)*np.sin(longitude)*eyp +
-              np.cos(self._colat)*ezp)
+        pv = (np.sin(colat)*np.cos(longitude)*exp +
+              np.sin(colat)*np.sin(longitude)*eyp +
+              np.cos(colat)*ezp)
 
         # renormalize and return pointing vector
         return pv / np.linalg.norm(pv)
diff --git a/phoebe/features/dataset_features.py b/phoebe/features/dataset_features.py
@@ -31,21 +31,41 @@ def __repr__(self):
     def get_parameters(self, **kwargs):
         raise NotImplementedError("get_parameters must be implemented in the feature subclass")
 
-    def modify_data_for_estimators(self, b, feature_ps, data_ps, **data_arrays):
+    def modify_data_for_estimators(self, b, data_ps, **data_arrays):
         """
         Modify the data parameters for the estimators.
         This is called before the data is passed to the estimators.
         """
         return {}
 
-    def modify_model(self, b, feature_ps, model_ps):
+    def modify_model(self, b, model_ps):
         raise NotImplementedError("modify_model must be implemented in the feature subclass")
 
 
-class Rv_Offset(DatasetFeature):
+class RVOffset(DatasetFeature):
     allowed_component_kinds = ['star']
     allowed_dataset_kinds = ['rv']
 
+    @classmethod
+    def create_feature_parameters(self, feature, **kwargs):
+        """
+        Create a <phoebe.parameters.ParameterSet> for an rvoffset feature.
+
+        Generally, this will be used as an input to the kind argument in
+        <phoebe.frontend.bundle.Bundle.add_feature>.  If attaching through
+        <phoebe.frontend.bundle.Bundle.add_feature>, all `**kwargs` will be
+        passed on to set the values as described in the arguments below.  Alternatively,
+        see <phoebe.parameters.ParameterSet.set_value> to set/change the values
+        after creating the Parameters.
+
+        Allowed to attach to:
+        * datasets: rv
+        """
+        params = []
+        params += [FloatParameter(qualifier='rv_offset', copy_for={'kind': ['star'], 'component': '*'}, component='_default', value=kwargs.get('rv_offset', 0.0), default_unit=u.km/u.s, description='Per-component offset to add to synthetic RVs (i.e. for hot stars)')]
+
+        return ParameterSet(params), []
+
     @classmethod
     def parse_bundle(cls, b, feature_ps):
         """
@@ -54,7 +74,7 @@ def parse_bundle(cls, b, feature_ps):
         rv_offsets = feature_ps.filter(qualifier='rv_offset', **_skip_filter_checks)
         return {param.component: param.get_quantity(**_skip_filter_checks) for param in rv_offsets.to_list()}
 
-    def modify_model(self, b, feature_ps, model_ps):
+    def modify_model(self, b, model_ps):
         for rv_param in model_ps.filter(qualifier='rvs', kind=['rv', 'mesh'], **_skip_filter_checks).to_list():
             rv_param.set_value(rv_param.get_value() + self.kwargs.get(rv_param.component).to_value(rv_param.default_unit), ignore_readonly=True, **_skip_filter_checks)
 
diff --git a/phoebe/frontend/bundle.py b/phoebe/frontend/bundle.py
@@ -17,7 +17,7 @@
 from packaging.version import parse
 from copy import deepcopy as _deepcopy
 import pickle as _pickle
-from inspect import getsource as _getsource
+from inspect import getsource as _getsource, isclass
 
 from scipy.optimize import curve_fit as cfit
 from tqdm import tqdm as _tqdm
@@ -42,6 +42,7 @@
 from phoebe.solverbackends import solverbackends as _solverbackends
 from phoebe.distortions import roche
 from phoebe.frontend import io
+from phoebe.features.common import BaseFeature
 from phoebe.features import dataset_features, component_features
 from phoebe.features.gaussian_processes import handle_gaussian_processes, _use_celerite2, _use_sklearn
 from phoebe.atmospheres.passbands import list_installed_passbands, list_online_passbands, get_passband, update_passband, _timestamp_to_dt
@@ -5599,8 +5600,8 @@ def add_feature(self, kind, component=None, dataset=None,
         * ValueError: if `dataset` is required but it not provided or is of the
             wrong kind.
         """
-        if getattr(kind, '_phoebe_custom_feature', False):
-            func = kind.get_parameters
+        if isclass(kind) and issubclass(kind, BaseFeature):
+            func = kind.create_feature_parameters
             kind_name = kind.__name__
             custom_feature = True
         else:
@@ -5645,6 +5646,7 @@ def add_feature(self, kind, component=None, dataset=None,
         if not _feature._dataset_allowed_for_feature(kind, dataset_kind):
             raise ValueError("{} does not support dataset with kind {}".format(kind_name, dataset_kind))
 
+        # NOTE: kwargs is guaranteed to include feature, which is a required argument to func
         params, constraints = func(**kwargs)
 
         if custom_feature:
@@ -5709,6 +5711,28 @@ def get_feature(self, feature=None, **kwargs):
         kwargs['context'] = 'feature'
         return self.filter(**kwargs)
 
+    def get_feature_code(self, feature=None, **kwargs):
+        """
+        Get the instantiated object containing the logic to run a feature.
+
+        See also:
+        * <phoebe.frontend.bundle.Bundle.get_feature>
+
+        Arguments
+        ---------
+        * `feature`: (string, optional, default=None): the name of the feature
+        * `**kwargs`: any other tags to do the filtering (excluding feature and context)
+
+        Returns:
+        * (obj): the object that implements the feature logic
+        """
+        feature_ps = self.get_feature(feature=feature, **kwargs)
+        if 'custom_code' in feature_ps.qualifiers:
+            cls = feature_ps.get_value(qualifier='custom_code', check_visible=False, check_default=False)
+        else:
+            cls = _feature._feature_classes.get(feature_ps.kind)
+        return cls.from_bundle(self, feature_ps)
+
     @send_if_client
     def remove_feature(self, feature=None, return_changes=False, **kwargs):
         """
@@ -12000,15 +12024,10 @@ def restore_conf():
                 # handle all dataset-features (except for GPs, which need to all be handled simultaneously
                 # and AFTER - instead of BEFORE - dataset-scaling from pblum_mode='dataset-scaled')
                 enabled_features = self.filter(qualifier='enabled', compute=compute, context='compute', value=True, **_skip_filter_checks).features
-                for feature in enabled_features:
-                    feature_ps = self.get_feature(feature=feature, **_skip_filter_checks)
-                    if feature_ps.get_value(qualifier='feature_type', **_skip_filter_checks) != 'dataset':
-                        continue
-                    if 'custom_code' in feature_ps.qualifiers:
-                        feature_cls = feature_ps.get_value(qualifier='custom_code', **_skip_filter_checks)
-                    else:
-                        feature_cls = getattr(dataset_features, feature_ps.kind.title(), None)
-                    feature_cls.from_bundle(self, feature_ps).modify_model(self, feature_ps, ml_params)
+                enabled_dataset_features = self.filter(qualifier='feature_type', feature=enabled_features, context='feature', value='dataset', **_skip_filter_checks).features
+                for feature in enabled_dataset_features:
+                    feature_obj = self.get_feature_code(feature=feature)
+                    feature_obj.modify_model(self, ml_params)
 
                 # handle flux scaling for any pblum_mode == 'dataset-scaled'
                 # or for any dataset in which pblum_mode == 'dataset-coupled' and pblum_dataset points to a 'dataset-scaled' dataset
diff --git a/phoebe/parameters/feature.py b/phoebe/parameters/feature.py
@@ -2,7 +2,8 @@
 
 from phoebe.parameters import *
 from phoebe.parameters import constraint
-from phoebe.features.dataset_features import *
+from phoebe.features.component_features import Spot
+from phoebe.features.dataset_features import RVOffset
 from phoebe import u
 from phoebe import conf
 
@@ -12,96 +13,31 @@
 
 ### NOTE: if creating new parameters, add to the _forbidden_labels list in parameters.py
 
-_allowed_components = {'spot': ['star', 'envelope'],
-                       'rv_offset': [None],
-                       'pulsation': ['star', 'envelope'],
+_allowed_components = {'pulsation': ['star', 'envelope'],
                        'gp_sklearn': [None],
-                       'gp_celerite2': [None],
-                       'gaussian_process': [None]}
+                       'gp_celerite2': [None]}
 
-_allowed_datasets = {'spot': [None],
-                     'rv_offset': ['rv'],
-                     'pulsation': [None],
+_allowed_datasets = {'pulsation': [None],
                      'gp_sklearn': ['lc', 'rv', 'lp'],
-                     'gp_celerite2': ['lc', 'rv', 'lp'],
-                     'gaussian_process': ['lc', 'rv', 'lp']} 
+                     'gp_celerite2': ['lc', 'rv', 'lp']} 
+
+_feature_classes = {}
 
 def _component_allowed_for_feature(feature_kind, component_kind):
     return component_kind in getattr(feature_kind, 'allowed_component_kinds', _allowed_components.get(feature_kind, []))
 
 def _dataset_allowed_for_feature(feature_kind, dataset_kind):
     return dataset_kind in getattr(feature_kind, 'allowed_dataset_kinds', _allowed_datasets.get(feature_kind, []))
 
-def _register(feature_cls, name):
-    globals()[name] = feature_cls.get_parameters
+def _register_feature(feature_cls, name):
+    globals()[name] = feature_cls.create_feature_parameters
     _allowed_components[name] = getattr(feature_cls, 'allowed_component_kinds', [])
     _allowed_datasets[name] = getattr(feature_cls, 'allowed_dataset_kinds', [])
+    _feature_classes[name] = feature_cls
 
-def spot(feature, **kwargs):
-    """
-    Create a <phoebe.parameters.ParameterSet> for a spot feature.
-
-    Generally, this will be used as an input to the kind argument in
-    <phoebe.frontend.bundle.Bundle.add_feature>.  If attaching through
-    <phoebe.frontend.bundle.Bundle.add_feature>, all `**kwargs` will be
-    passed on to set the values as described in the arguments below.  Alternatively,
-    see <phoebe.parameters.ParameterSet.set_value> to set/change the values
-    after creating the Parameters.
-
-    Allowed to attach to:
-    * components with kind: star
-    * datasets: not allowed
-
-    Arguments
-    ----------
-    * `colat` (float/quantity, optional): colatitude of the center of the spot
-        wrt spin axis.
-    * `long` (float/quantity, optional): longitude of the center of the spot wrt
-        spin axis.
-    * `radius` (float/quantity, optional): angular radius of the spot.
-    * `relteff` (float/quantity, optional): temperature of the spot relative
-        to the intrinsic temperature.
-
-    Returns
-    --------
-    * (<phoebe.parameters.ParameterSet>, list): ParameterSet of all newly created
-        <phoebe.parameters.Parameter> objects and a list of all necessary
-        constraints.
-    """
 
-    params = []
-
-    params += [FloatParameter(qualifier="colat", value=kwargs.get('colat', 0.0), default_unit=u.deg, description='Colatitude of the center of the spot wrt spin axis')]
-    params += [FloatParameter(qualifier="long", value=kwargs.get('long', 0.0), default_unit=u.deg, description='Longitude of the center of the spot wrt spin axis')]
-    params += [FloatParameter(qualifier='radius', value=kwargs.get('radius', 1.0), default_unit=u.deg, description='Angular radius of the spot')]
-    # params += [FloatParameter(qualifier='area', value=kwargs.get('area', 1.0), default_unit=u.solRad, description='Surface area of the spot')]
-
-    params += [FloatParameter(qualifier='relteff', value=kwargs.get('relteff', 1.0), limits=(0.,None), default_unit=u.dimensionless_unscaled, description='Temperature of the spot relative to the intrinsic temperature')]
-    # params += [FloatParameter(qualifier='teff', value=kwargs.get('teff', 10000), default_unit=u.K, description='Temperature of the spot')]
-
-    constraints = []
-
-    return ParameterSet(params), constraints
-
-def rv_offset(feature, **kwargs):
-    """
-    Create a <phoebe.parameters.ParameterSet> for an rvoffset feature.
-
-    Generally, this will be used as an input to the kind argument in
-    <phoebe.frontend.bundle.Bundle.add_feature>.  If attaching through
-    <phoebe.frontend.bundle.Bundle.add_feature>, all `**kwargs` will be
-    passed on to set the values as described in the arguments below.  Alternatively,
-    see <phoebe.parameters.ParameterSet.set_value> to set/change the values
-    after creating the Parameters.
-
-    Allowed to attach to:
-    * datasets: rv
-    """
-    params = []
-
-    params += [FloatParameter(qualifier='rv_offset', copy_for={'kind': ['star'], 'component': '*'}, component='_default', value=kwargs.get('rv_offset', 0.0), default_unit=u.km/u.s, description='Per-component offset to add to synthetic RVs (i.e. for hot stars)')]
-
-    return ParameterSet(params), []
+_register_feature(Spot, 'spot')
+_register_feature(RVOffset, 'rv_offset')
 
 
 def gp_sklearn(feature, **kwargs):
@@ -270,13 +206,3 @@ def gp_celerite2(feature, **kwargs):
     constraints = []
 
     return ParameterSet(params), constraints
-
-def gaussian_process(feature, **kwargs):
-    """
-    Deprecated (will be removed in PHOEBE 2.5)
-
-    Support for celerite gaussian processes has been removed.  This is now an
-    alias to <phoebe.parameters.feature.gp_celerite2>.
-    """
-    logger.warning("gaussian_process is deprecated.  Use gp_celerite2 instead.")
-    return gp_celerite2(feature, **kwargs)
diff --git a/phoebe/solverbackends/solverbackends.py b/phoebe/solverbackends/solverbackends.py
@@ -246,22 +246,17 @@ def _get_combined_lc(b, datasets, combine, phase_component=None, mask=True, norm
             ds_sigmas = np.full_like(ds_fluxes, fill_value=0.001*ds_fluxes.mean())
 
         # run dataset features attached to this dataset
-        for feature in b.filter(context='feature', dataset=dataset, **_skip_filter_checks).features:
-            feature_ps = b.get_feature(feature=feature, **_skip_filter_checks)
-            if feature_ps.get_value(qualifier='feature_type', **_skip_filter_checks) != 'dataset':
-                continue
-            if 'custom_code' in feature_ps.qualifiers:
-                feature_cls = feature_ps.get_value(qualifier='custom_code', **_skip_filter_checks)
-            else:
-                # TODO: import
-                feature_cls = getattr(dataset_features, feature_ps.kind.title(), None)
-            if feature_cls is None:
-                raise ValueError("dataset feature '{}' not found".format(feature_ps.kind))
-            modified_params = feature_cls.from_bundle(b, feature).modify_data_for_estimators(b, feature_ps,
-                                                                                             lc_ps,
-                                                                                             times=ds_times,
-                                                                                             fluxes=ds_fluxes,
-                                                                                             sigmas=ds_sigmas)
+        # NOTE: enabled features are per-compute but estimators don't even have a compute
+        # so instead we just include any that are enabled in ANY compute
+        # TODO: copy enabled@feature for all estimators (but not other solvers)
+        enabled_features = b.filter(qualifier='enabled', value=True, **_skip_filter_checks).features
+        for feature in b.filter(context='feature', feature=enabled_features, dataset=dataset, **_skip_filter_checks).features:
+            feature_obj = b.get_feature_code(feature=feature)
+            modified_params = feature_obj.modify_data_for_estimators(b,
+                                                                     lc_ps,
+                                                                     times=ds_times,
+                                                                     fluxes=ds_fluxes,
+                                                                     sigmas=ds_sigmas)
             ds_times = modified_params.get('times', ds_times)
             ds_fluxes = modified_params.get('fluxes', ds_fluxes)
             ds_sigmas = modified_params.get('sigmas', ds_sigmas)
