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

Fix constrained optimizer documentation #201

Merged
merged 8 commits into from Jul 26, 2020

Conversation

@rcurtin
Copy link
Member

@rcurtin rcurtin commented Jun 23, 2020

This solves #199.

Our documentation for constrained functions is actually incorrect. The documentation implies that both "hard" and "soft" constraints are supported but this is not true. In fact, the augmented Lagrangian optimizer (the only constrained optimizer we currently have) requires "soft" (differentiable) constraints. Although I think it would be useful to provide support for optimizers that can take in arbitrary constraints, at the moment we don't have any, so I thought it was best to just simplify the documentation.

Also I fixed an issue where the L-BFGS optimizer will return NaNs when the starting point gives a gradient that is 0. Essentially, for some reason, we previously did not allow termination on the first iteration due to the gradient norm being too small. That's likely just a code artifact; I can't find any good reason to avoid that.

@birm
Copy link
Member

@birm birm commented Jun 23, 2020

It looks like we need a corresponding mlpack pr to fix some dependent tests too?

@zoq
Copy link
Member

@zoq zoq commented Jun 23, 2020

It looks like we need a corresponding mlpack pr to fix some dependent tests too?

This shouldn't break anything or require any update.

@rcurtin
Copy link
Member Author

@rcurtin rcurtin commented Jun 23, 2020

No, actually, it looks like mlpack's NCA implementation may depend on L-BFGS not terminating in the first iteration. (I am not clear on why the memory checks or static code analysis jobs are failing...)

@zoq
Copy link
Member

@zoq zoq commented Jun 23, 2020

I am not clear on why the memory checks or static code analysis jobs are failing...

I guess for the static analysis job we have to update the job config same we did for the mlpack one, will look into it now.

@zoq
Copy link
Member

@zoq zoq commented Jun 23, 2020

@mlpack-jenkins test this please

2 similar comments
@zoq
Copy link
Member

@zoq zoq commented Jun 23, 2020

@mlpack-jenkins test this please

@zoq
Copy link
Member

@zoq zoq commented Jun 23, 2020

@mlpack-jenkins test this please

@zoq
Copy link
Member

@zoq zoq commented Jun 23, 2020

The static code analysis check works now.

@rcurtin
Copy link
Member Author

@rcurtin rcurtin commented Jun 28, 2020

Now that mlpack/mlpack#2479 is merged, this should build okay... let's find out...

@rcurtin
Copy link
Member Author

@rcurtin rcurtin commented Jun 28, 2020

@mlpack-jenkins test this please

@rcurtin
Copy link
Member Author

@rcurtin rcurtin commented Jun 29, 2020

@mlpack-jenkins test this please

2 similar comments
@rcurtin
Copy link
Member Author

@rcurtin rcurtin commented Jul 5, 2020

@mlpack-jenkins test this please

@rcurtin
Copy link
Member Author

@rcurtin rcurtin commented Jul 6, 2020

@mlpack-jenkins test this please

@rcurtin
Copy link
Member Author

@rcurtin rcurtin commented Jul 6, 2020

@zoq the memory checks build times out, even after 6 hours, I think because it's trying to run all of the tests---do you think we should adapt the script in jenkins-conf/memory/parse-tests.py to the ensmallen codebase? I am not familiar with that script, but I'm happy to give it a shot if you like. 👍

@rcurtin
Copy link
Member Author

@rcurtin rcurtin commented Jul 25, 2020

Anyone willing to review this one? I believe everything is working here and we could merge it. 👍

@zoq
Copy link
Member

@zoq zoq commented Jul 25, 2020

@zoq the memory checks build times out, even after 6 hours, I think because it's trying to run all of the tests---do you think we should adapt the script in jenkins-conf/memory/parse-tests.py to the ensmallen codebase? I am not familiar with that script, but I'm happy to give it a shot if you like. 👍

I missed this one, I'll take a look at the memory script later, should be easy, but don't think we have to wait for the memory job to pass.

@zoq
Copy link
Member

@zoq zoq commented Jul 25, 2020

Just resolved the merge conflict.

@zoq
zoq approved these changes Jul 25, 2020
Copy link
Member

@zoq zoq left a comment

Looks great to me.

@mlpack-bot
mlpack-bot bot approved these changes Jul 26, 2020
Copy link

@mlpack-bot mlpack-bot bot left a comment

Second approval provided automatically after 24 hours. 👍

@zoq zoq merged commit 8c98f83 into mlpack:master Jul 26, 2020
4 of 5 checks passed
4 of 5 checks passed
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
mlpack master build test Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.