Skip to content

Introduce Design configuration API - #214

Closed
leostimpfle wants to merge 8 commits into
py-econometrics:mainfrom
leostimpfle:feat/design-row-mapping
Closed

Introduce Design configuration API#214
leostimpfle wants to merge 8 commits into
py-econometrics:mainfrom
leostimpfle:feat/design-row-mapping

Conversation

@leostimpfle

Copy link
Copy Markdown
Contributor

This is a first stab at #47 with the goal of keeping the API changes generic.

The main change from the API side is the introduction of DesignOptions for configuration of Design::build. It currently only holds two boolean flags drop_singletons (public) and locality_sort (crate internal) but should in principle be easy to extend.

The main functional change is that Design:obs_perm is replaced by a generic mapping Design::rows which maps the raw input data to the internal state (after dropping singletons and locality sorting).

@schroedk This is early stage and I will keep working on it before converting from a draft PR but any feedback you might have would be much appreciated.

@leostimpfle leostimpfle changed the title Feat/design row mapping Introduce Design configuration API Jul 26, 2026
@leostimpfle

Copy link
Copy Markdown
Contributor Author

Commit 37de838 adds weights to DesignOptions. This is absolutely not necessary but I think it nicely tightens the Design construction.

@leostimpfle
leostimpfle force-pushed the feat/design-row-mapping branch from c1ac061 to b882362 Compare July 31, 2026 09:11
@schroedk

schroedk commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

@leostimpfle thanks for taking this:) I am sorry, I merged a bigger refactoring so you had to resolve conflicts:(
And I am sorry for the late answer.
Introducing a DesignOptions parameter keeps a single entry point, but also has some drawbacks:

  • in this impl, the caller loses the possibility to reconstruct a full vector (the reverse map is missing)
  • dropping is more like a preprocess and I would not mix it together with the demeaning itself.
  • I would expect performance reduction due to the indexing indirection via the row map

As alternative, I was thinking about a purely additive solution. We could introduce a new type ReducedDesign and just a free function drop_singletons: Design -> ReducedDesign. ReducedDesign implements IntoDesign and owns the reverse mapping logic. Something like this:

/// Rows dropped, level ids compacted, plus the maps back to caller indexing.
  pub struct ReducedDesign { /* … */ }

  pub fn drop_singletons(design: &Design<'_>) -> Result<ReducedDesign, BuildError>;

  impl ReducedDesign {
      /// Caller order -> reduced order, for `y` or weights.
      pub fn to_reduced<T: Clone>(&self, full: &[T]) -> Vec<T>;
  }

  impl<'a> IntoDesign<'a> for &'a ReducedDesign {}

  /// Translate a reduced-space result back to the caller's row and level indexing.
  pub trait ToFull {
      /// Consumes the reduced result; dropped slots get `fill`.
      fn to_full(self, reduced: &ReducedDesign, fill: f64) -> Self;
  }

  impl ToFull for SolveResult {}
  impl ToFull for BatchSolveResult {}

  let reduced = drop_singletons(&design)?;
  let result = solve(&reduced, &reduced.to_reduced(&y), None, &lsmr, None)?
      .to_full(&reduced, f64::NAN);

This is more a lower level api. For me this feels more natural as the nature of this library os more like a demean backend and not a full regression library (what made me question adding the singleton drop at all from the very beginning). What do you think?

@leostimpfle

Copy link
Copy Markdown
Contributor Author

Hi @schroedk! No problem and thanks for the feedback.

I initially thought of something akin to what you're describing (i.e., implementing processing methods on Design) but gravitated towards DesignOptions as a construction policy for the persistent solver API. My main motivation was to establish guarantees at solver construction that exactly describe the demeaning so that various right-hand sides can be solved ergonomically. I worry that transformations of the form Design to ReducedDesign implementing IntoDesign would make it potentially much harder to keep track of what design a Solver is actually solving. (I think that a similar argument can be made for weights which I have therefore added to DesignOptions in ee5317b.)


To answer your points more specifically:

in this impl, the caller loses the possibility to reconstruct a full vector (the reverse map is missing)

Maybe I'm missing the point but my idea was that the internal data representation is hidden inside Design which translates for callers via to_internal_row_order and to_input_row_order. The idea was precisely to have an immutable Design that allows a persistent solver to ingest various right-hand sides repeatedly (whose rows corresponding to singleton observations need to be correctly accounted for). Replicating this with ReducedDesign::IntoDesign might become cumbersome because the mapping has to be stored somewhere?

dropping is more like a preprocess and I would not mix it together with the demeaning itself

Agreed, but it does affect the solution a Solver returns. In view of the persistent solver API, I was thinking that all properties of a Design that affect the solution should therefore be auditable in some way.

I would expect performance reduction due to the indexing indirection via the row map

The row ordering may create some overhead compared to the pure locality sort but it is a one-off cost (per RHS). My (very superficial) understanding is that there is no row-map indirection inside the LSMR iterations which I would expect to dominate runtime?


Separately but somewhat relatedly: While rebasing I noticed your dense versus sparse sorting introduced in #221. My current implementation doesn't take advantage of it singleton detection but the issue applies more generally across the codebase which I believe is currently not addressed consistently, for example for the coefficient field SolveResult::x. I'm wondering if we should think about a compactification of sparse factor codes? (Maybe best to do open a separate issue to avoid cluttering this PR?)

Sorry for the long reply. Let me know if it would be easier to chat on a call.

@leostimpfle
leostimpfle force-pushed the feat/design-row-mapping branch from 22a6677 to b2da9a2 Compare July 31, 2026 10:51
@leostimpfle
leostimpfle marked this pull request as ready for review July 31, 2026 11:07
@leostimpfle

Copy link
Copy Markdown
Contributor Author

Closing this as we might redesign the encoding boundary for within as discussed with @schroedk

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants