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

Add more general linear operator constraint to sdp #350

Conversation

SuvarshaChennareddy
Copy link
Contributor

Hello everyone,
This is related to issue #72. I've allowed the ability to store constraints as std::function objects.
I have not modified/added tests yet. However, if this PR looks alright, I'll be willing to do so.
Any comments would be greatly appreciated.

@mlpack-bot
Copy link

mlpack-bot bot commented Dec 10, 2022

Thanks for opening your first pull request in this repository! Someone will review it when they have a chance. In the mean time, please be sure that you've handled the following things, to make the review process quicker and easier:

  • All code should follow the style guide
  • Documentation added for any new functionality
  • Tests added for any new functionality
  • Tests that are added follow the testing guide
  • Headers and license information added to the top of any new code files
  • HISTORY.md updated if the changes are big or user-facing
  • All CI checks should be passing

Thank you again for your contributions! 👍

Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

Hey @SuvarshaChennareddy, thanks for taking the time to look into this. I have some basic comments before you proceed further. I think that tests are definitely necessary both to ensure that the implementation is correct, and also that it might behave like a user expects.

If I were you, I would start by assembling a couple reasonable examples of how a user might want to express a constraint in this way, and then designing to make those cases easy. 👍

@@ -109,6 +141,8 @@ inline void Smat(const MatAType& input, MatBType& output)
}
}



Copy link
Member

Choose a reason for hiding this comment

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

No need to add this whitespace :)

include/ensmallen_bits/sdp/lrsdp_function.hpp Outdated Show resolved Hide resolved
*/
template<typename ElemType, typename MatBType>
inline void convertToMatrix(std::function<ElemType(arma::Mat<ElemType>)> input,
MatBType& output)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the approach you are taking is to turn a std::function that the user gives into a matrix, and then use that as a constraint. But this is the opposite of what we are looking for in #72: take the given example of the constraint that the sum of elements is a constant. You can express this very tediously as the constraint trace(1 1^T X) = b, but the much easier way to represent this is sum(X) = b. So, instead of extracting the matrix 1 1^T, it would be better to just store the std::functions in the SDP objects and evaluate them directly to get the value.

Copy link
Contributor Author

@SuvarshaChennareddy SuvarshaChennareddy Dec 15, 2022

Choose a reason for hiding this comment

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

First of all, thank you for taking the time to look into this. I'm glad you asked this. I honestly didn't know if this would be alright.

However, (from what I understand) in the interior point method, the constraints need to be expressed as matrices (as matrix multiplication and transposing is involved).

Although, in the LRSDP implementation, I have used these std::functions wherever the matrix conversion isn't necessary (again like in the case of matrix multiplication or transposing).

I had given it some thought but using these std::functions directly would be extremely tedious as a lot of factors must be accounted for. For example, how would I calculate M=A(E^-1)F(A^T) where A is the matrix of vector forms of the constraints.

Nevertheless, I will think about this a bit more and try to come with a more appropriate solution.

@SuvarshaChennareddy
Copy link
Contributor Author

Will do! Thank you!

@mlpack-bot
Copy link

mlpack-bot bot commented Jan 14, 2023

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍

@mlpack-bot mlpack-bot bot added the s: stale label Jan 14, 2023
@mlpack-bot mlpack-bot bot closed this Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants