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

streamline OrdinalMarginLoss construction #89

Merged
merged 1 commit into from
Jul 13, 2017
Merged

streamline OrdinalMarginLoss construction #89

merged 1 commit into from
Jul 13, 2017

Conversation

Evizero
Copy link
Member

@Evizero Evizero commented Jul 10, 2017

Since we ended up not doing the unrolling, it seems more flexible to not make N a type var. This way its cleaner for N to depend on the data.

Aside from this, I wrote the constructor of OrdinalMarginLoss such that its easy to define clean typealiases.

For example the following is possible:

const OrdinalHingeLoss = OrdinalMarginLoss{HingeLoss}
OrdinalHingeLoss(5) # N=5

Since HingeLoss has no free parameters the above is quite simple, but the construction even works for all MarginLoss that do have free paramaters, such as the SmoothedL1HingeLoss

julia> const OrdinalSmoothedHingeLoss = OrdinalMarginLoss{<:SmoothedL1HingeLoss};

julia> OrdinalSmoothedHingeLoss(5, 2.1)
OrdinalMarginLoss{SmoothedL1HingeLoss{Float64}}(SmoothedL1HingeLoss{Float64}(2.1), 4)

cc: @mihirparadkar

@mihirparadkar
Copy link

That seems like a good idea for ease-of-use. My one possible concern with removing the type parameter is that the compiler may have been unrolling the loop already but can't anymore, but this doesn't seem like a big deal for a small number of levels.

@Evizero
Copy link
Member Author

Evizero commented Jul 13, 2017

but this doesn't seem like a big deal for a small number of levels.

The thing is if it doesn't really help with for a small loop, it is unlikely to help with a large one.

Regardless, I am far from opposed to changing it back at some point if some benchmarks show its benefit. All I really aim for here is to not make any non-trivial optimization that didn't show promise.
And without the unrolling there is currently no upside to having the number of levels in the type

@Evizero Evizero merged commit 9f45628 into master Jul 13, 2017
@Evizero Evizero deleted the cs/ordinal branch July 13, 2017 07:57
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.

None yet

2 participants