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

Letor updates #324

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Letor updates #324

wants to merge 5 commits into from

Conversation

ojwb
Copy link
Contributor

@ojwb ojwb commented Jun 29, 2022

This is a replacement for #320, updated for the change from travis to GHA.

@ojwb
Copy link
Contributor Author

ojwb commented Jun 29, 2022

I've rebased onto current master, which was clean apart from .travis.yml having been removed.

@ojwb
Copy link
Contributor Author

ojwb commented Jun 30, 2022

We are still seeing very similar looking test failures in the linux clang build and on macos (macos uses clang as the default compiler so these may well be the same issue).

I've tested with clang (with ubsan and asan enabled) locally and have a clean test run (after a few fixes for merge problems and one bug).

I think I'm going to try to peel out logically separate changes and test and merge them separately to try to get to the bottom of this. I've already cherry-picked and merged the changes to drop SVMRanker.

I'll likely rebase and force-push this branch to drop out merged changes. If anyone wants to help with this, please speak up and we can come up with a plan.

This change applies to both ListNetRanker and ListMleRanker.
The motivation for Xavier initialization in Neural Networks is to
initialize the weights of the network so that the neuron activation
functions are not starting out in saturated or dead regions. In other
words, we want to initialize the parameters with random values that are
not “too small” and not “too large.”
In effect, a bias value allows you to shift the activation function
to the left or right, which may be critical for successful learning.
This are due to changes made from fixing ranker implementations, fixing
indexing errors, adding Xavier initialisation, adding normalisation of
gradient and adding bias combined. This also fixes scorer test which was
wrong earlier.
Hardwire it to what default_random_engine gives with current GCC,
since that passes the test suite.

Maybe this is why some CI builds fail some tests...
@ojwb
Copy link
Contributor Author

ojwb commented Jul 1, 2022

I've merged several commits to master, and have reached the point where I can't easily peel off more because there are 3 commits which change the testsuite results, and one commit which updates the testsuite to pass again.

I noticed the Xavier initialisation was using default_random_engine which may vary between implementations so I tried changing it to minstd_rand0 as that's what default_random_engine actually is with current GCC.

That's helped, but there's still one test failing with clang and on macos, but passing elsewhere (and for me locally with clang).

There are also two XFAIL-ed testcases passing (which I don't see locally either):

./apitest total: 81 tests passed, 1 failed, 2 expected failures passed, 6 expected failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants