Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rectified MVE class #385

Merged
merged 36 commits into from
Feb 27, 2024
Merged

Rectified MVE class #385

merged 36 commits into from
Feb 27, 2024

Conversation

dannys4
Copy link
Contributor

@dannys4 dannys4 commented Feb 5, 2024

Exactly what it sounds like; a class, as described in previous PRs, to define a function $T:\mathbb{R}^d\times\mathbb{R}\to\mathbb{R}$ which performs
$$T(\mathbf{x},y) = \sum_{\vec{\alpha}^1_x} c^1_{\vec{\alpha}^1_x} \Psi_{\vec{\alpha}^1_x}(\mathbf{x})+ \sum_{\vec{\alpha}^2}c^2_{\vec{\alpha}^2}r\left[\Psi_{\vec{\alpha}^2_x}(\mathbf{x})\right]g_{\alpha^2_y}(y),$$

where $\vec{\alpha}^1_x$ and $\vec{\alpha}^2$ do not necessarily come from the same multi-index set (indeed, we want all $\vec{\alpha}^2$ to come from a set that is exclusively non-constant in $y$ to avoid redundant terms); further, all ${g_j}$ should be monotone, and $r:\mathbb{R}\to\mathbb{R}^+$ should probably be bijective (not sure how necessary that is though), and the second set of coefficients, ${c^2_{\vec{\alpha}_2}}$, must be nonnegative.

This went through several iterations and I think this is the least bad choice here. The provided MVE would've worked to some degree, but defining the LogDet and Inverse functions would've been invasive, and we'd lose some efficiency from spawning more parallelism than necessary (as well as the fact that MVE is engineered to give many outputs simultaneously, whereas this is a special case).

Bindings and a MapFactory method will be created in a future PR.

@dannys4 dannys4 requested a review from mparno February 21, 2024 13:11
@dannys4 dannys4 marked this pull request as ready for review February 21, 2024 13:11
Copy link
Contributor

@mparno mparno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Danny! I have one comment about the use of virtual keywords, but everything of real substance looks great! It'll be great to have this in MParT.

MParT/DerivativeFlags.h Show resolved Hide resolved
virtual void GradientImpl(StridedMatrix<const double, MemorySpace> const& pts,
StridedMatrix<const double, MemorySpace> const& sens,
StridedMatrix<double, MemorySpace> output) override
void GradientImpl(StridedMatrix<const double, MemorySpace> const& pts,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While not strictly necessary, I would advocate for keeping the virtual keyword across all of the *Impl classes. It avoids some compiler warnings and, even though we have the override keyword, makes it more obvious that these functions stem from the base class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what compiler warnings this avoids, as any non-final function that overrides is, by definition, a virtual function (I haven't bothered to look it up in the actual standard, but it's on the cppref virtual site). That being said, the difference is minimal and I'm more than happy to undo this change.

MParT/Utilities/RootFinding.h Show resolved Hide resolved
@dannys4 dannys4 requested a review from mparno February 26, 2024 20:20
Copy link
Contributor

@mparno mparno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Danny!

@mparno mparno merged commit a12641b into MeasureTransport:main Feb 27, 2024
5 checks passed
@dannys4 dannys4 deleted the RectifiedMVEClass branch February 27, 2024 20:37
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