Skip to content

Conversation

@ashelkovnykov
Copy link
Contributor

  • Experimental results when comparing Smoothed Hinge Loss to Logistic Regression were substantially worse and also increased training time significantly due to slower convergence
  • Removed GLMLossFunction and derived classes, since all training tasks in Photon ML are GLM tasks

Alex Shelkovnykov and others added 2 commits May 7, 2020 11:53
- Experimental results when comparing Smoothed Hinge Loss to Logistic Regression were substantially worse and also reduced training time significantly due to slower convergence
- Removed GLMLossFunction and derived classes, since all training tasks in Photon ML are GLM tasks
@ashelkovnykov
Copy link
Contributor Author

@cmjiang @yunboouyang @lguo
Updated code for latest master, requesting review

Copy link
Collaborator

@yunboouyang yunboouyang left a comment

Choose a reason for hiding this comment

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

LGTM

classOf[GeneralizedLinearModel],
classOf[GeneralizedLinearOptimizationProblem[_]],
classOf[GLMOptimizationConfiguration],
classOf[HessianDiagonalAggregator],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to add this 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 noticed that we had the ValueAndGradientAggregator and HessianVectorAggregator being registered with Kryo, but not the HessianDiagonalAggregator or the HessianMatrixAggregator. Since all four function the same way, it stands to reason that they should either all be registered or none of them be registered. Since objects of these four class types will for sure be distributed, therefore they should all be registered.

Apparently, Kryo was able to figure this out on its own, but I think it best to make it explicit.

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