-
Notifications
You must be signed in to change notification settings - Fork 4
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
Rectified MVE class #385
Conversation
There was a problem hiding this 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/MultivariateExpansion.h
Outdated
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 override
s 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Danny!
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.