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

Providing option to calculate exact objective #109

Merged
merged 17 commits into from Aug 8, 2019

Conversation

@walragatver
Copy link
Contributor

commented Apr 28, 2019

Hi, this is for #108. It looks like we need to modify some tests in the mlpack repository after this change. I will start working on it once this gets approved.

@mlpack-bot

This comment has been minimized.

Copy link

commented Apr 28, 2019

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! 👍

@@ -64,6 +64,9 @@ class AdaDelta
* @param tolerance Maximum absolute tolerance to terminate algorithm.
* @param shuffle If true, the function order is shuffled; otherwise, each
* function is visited in linear order.
* @param exactObjective Flag that determines whether actual objective over

This comment has been minimized.

Copy link
@zoq

zoq May 10, 2019

Member

Sounds a little bit vague to me, what about "Calculate the exact objective (Default: estimate the final objective obtained on the last pass over the data)."

This comment has been minimized.

Copy link
@walragatver

walragatver May 11, 2019

Author Contributor

Yes, this is quite clear.

@@ -128,6 +132,11 @@ class AdaDelta
//! Modify whether or not the individual functions are shuffled.
bool& Shuffle() { return optimizer.Shuffle(); }

//! Get whether or not the actual objective is calculated after training.

This comment has been minimized.

Copy link
@zoq

zoq May 10, 2019

Member

Not sure, I would probably remove the "after training" part, see this more as an optimisation process.

This comment has been minimized.

Copy link
@walragatver

walragatver May 11, 2019

Author Contributor

Sorry, I just got carried away.

@rcurtin
Copy link
Member

left a comment

Hey @walragatver, this is looking great. Thanks for taking the time to do these changes. It looks like there are a few optimizers where we didn't need to make any changes, and also, before we merge this we should update HISTORY.md and doc/optimizers.md to reflect the changes. The documentation changes will be a bit tedious but formulaic. Let me know if I can clarify any comments. Thanks! 👍

{
lastBestFitness = function.Evaluate(iterate);
}
return lastBestFitness;

This comment has been minimized.

Copy link
@rcurtin

rcurtin May 12, 2019

Member

Since CNE uses only an arbitrary function type, there's no need for exactObjective here. It only applies to optimizers that optimize decomposable functions. I think the documentation is a little wrong for CNE though (I updated it in another PR that I'm about to open).

{
lastObjective = function.Evaluate(iterate);
}
return lastObjective;

This comment has been minimized.

Copy link
@rcurtin

rcurtin May 12, 2019

Member

Just like CNE, SCD doesn't work on decomposable functions, so I think we can leave SCD as it is. 👍

{
overallObjective = function.Evaluate(iterate);
}
return overallObjective;

This comment has been minimized.

Copy link
@rcurtin

rcurtin May 12, 2019

Member

SPSA doesn't work on decomposable functions, so we can avoid the change here too.

@walragatver walragatver force-pushed the walragatver:exact-objective branch 3 times, most recently from 2bea01e to 4e1c144 May 12, 2019

@walragatver

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

Hi @rcurtin , @zoq ,
This is now ready to merge. I have currently kept tommorow's date in History.md.

@walragatver walragatver force-pushed the walragatver:exact-objective branch 2 times, most recently from 8a6bc23 to 25f3f78 May 17, 2019

@zoq zoq removed the s: unanswered label Jun 11, 2019

@zoq
Copy link
Member

left a comment

Sorry for the slow response, just commented on a really minor issue; if you can fix the issue I think this is ready to be merged.

HISTORY.md Outdated
@@ -1,3 +1,7 @@
### ensmallen 1.1?.?
###### ????-??-??
* Add option to avoid computing exact objective at end of optimization (#109).

This comment has been minimized.

Copy link
@zoq

zoq Jun 11, 2019

Member

Super picky comment: "at the end of the optimization".

| `bool` | **`resetPolicy`** | If true, parameters are reset before every Optimize call; otherwise, their values are retained. | `true` |

The attributes of the optimizer may also be modified via the member methods
`StepSize()`, `BatchSize()`, `Beta1()`, `Beta2()`, `Eps()`, `MaxIterations()`,
`Tolerance()`, `Shuffle()`,`V1()`,`V2()`, and `ResetPolicy()`.
`Tolerance()`, `Shuffle()`, `ExactObjective()`, `V1()`, `V2()`, and `ResetPolicy()`.

This comment has been minimized.

Copy link
@zoq

zoq Jun 11, 2019

Member

Nice catch!

@walragatver walragatver force-pushed the walragatver:exact-objective branch from 4d68444 to 1a25b58 Jun 11, 2019

@walragatver

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Hi @zoq,
Thanks for the final approval. I have made the required changes.

@coatless

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

Regarding the parameter ordering, the current function call is:

AdaDelta(stepSize, batchSize, rho, epsilon, maxIterations, tolerance, shuffle, resetPolicy)

In this update, the function call switches to:

AdaDelta(stepSize, batchSize, rho, epsilon, maxIterations, tolerance, shuffle, **exactObjective**, resetPolicy)

Perhaps the addition of exactObjective should be at the end of the function definition?

AdaDelta(stepSize, batchSize, rho, epsilon, maxIterations, tolerance, shuffle, resetPolicy, **exactObjective**)
@walragatver

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@coatless, Thanks for raising the issue. I also wanted to discuss that. If you look in some of the cases UpdateRule parameter comes after the shuffle parameter. If we keep exactObjective after that then in order to use it a user will need to provide the updateRule parameter always. I thought it's not a great idea. So I kept exactObjective after shuffle as both are bools indicating some optional functionality.
Let me know if this is incorrect.

@zoq

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Good point, I think I rather change the update policies instead of changing the behavior of the objective calculation. So personally I would add the value at the end. Note this will be part of the next major release so we don't necessarily have to worry about backward compatibility, but I think it would be great to have anyway.

@mlpack-bot

This comment has been minimized.

Copy link

commented Jul 11, 2019

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 Jul 11, 2019

@zoq zoq added s: keep open and removed s: stale labels Jul 12, 2019

@zoq

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

Let us keep this open, I think this is close to being merged.

@rcurtin

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

It seems to me like the only thing that remains is to fix the argument ordering issue. I agree with @zoq on what he wrote:

Good point, I think I rather change the update policies instead of changing the behavior of the objective calculation. So personally I would add the value at the end.

@walragatver walragatver force-pushed the walragatver:exact-objective branch from 1a25b58 to c1a7fa6 Jul 31, 2019

@walragatver

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Hi @rcurtin,
Sorry for the slow response. Instead of adding a new constructor I have added exactObjective to the last position. I did that because it was difficult to overload the constructor because the last three arguments are of same type(bool). I have modified everything and this is ready to merge.

@zoq

zoq approved these changes Aug 1, 2019

Copy link
Member

left a comment

Thanks for working on this, I think it would make sense to test if the number of Evaluatecalls are different if used with exactObjective = true, but it's probably easier to do once the Callbacks PR is merged.

@mlpack-bot

mlpack-bot bot approved these changes Aug 2, 2019

Copy link

left a comment

Second approval provided automatically after 24 hours. 👍

@rcurtin rcurtin merged commit ca48812 into mlpack:master Aug 8, 2019

2 checks passed

Static Code Analysis Checks Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rcurtin

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@walragatver awesome work, thanks again! 👍

@walragatver walragatver deleted the walragatver:exact-objective branch Aug 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.