Skip to content
This repository has been archived by the owner on Apr 24, 2024. It is now read-only.

Bug fixes in ridge tests and reduces dependency on solver #30

Merged
merged 2 commits into from
Mar 7, 2023

Conversation

agoscinski
Copy link
Collaborator

@agoscinski agoscinski commented Mar 4, 2023

first commit is fixing several issues with ridge tests:

  • the seed for rng was just set once, that means changing the order of the tests changed the tested matrices in the tests which made debugging hard
  • In the tests sometimes np.random.normal was used, but to use the seed one has to use self.rng.normal
  • In test test_consistent_weights_scaling the regularization was multiplied with a zero array always, so I removed the parameter and just use zero now
  • move documentation of tests

second commit is reducing the dependency on the type of solver by:

  • changing tests to
    • Tests without regularization should be high accurate wrt w_exact as long as samples >> rank(X)
    • Tests with regularization should be highly accurate between solvers if num properties is very small otherwise only approximation

second commit also unifies accuracies

  • approx: atol=1e-13, rtol=1e-8
  • high accuray: atol=1e-13, rtol=1e-8

📚 Documentation preview 📚: https://equisolve--30.org.readthedocs.build/en/30/

Copy link
Collaborator

@PicoCentauri PicoCentauri left a comment

Choose a reason for hiding this comment

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

Thanks Alex for taking care of these tests! I have just a minor comment but they are already MUCH cleaner!

* the seed for rng was just set once, added autouse fixture to set it
  before each test

* replace np.random.normal with self.rng.normal to use seed

* scaling tests don't use regularization, so it was removed

* move documentation to start of tests
* Tests without regularization should be high accruate wrt. w_exact as
  long as samples >> rank(X)

* Tests with regularization should be highly accurate between solvers if
  num_properties is very small otherwise only approximative

* unifies accuracies:
  - approx: atol=1e-13, rtol=1e-8
  - high accuray: atol=1e-15, rtol=1e-10
@agoscinski agoscinski marked this pull request as ready for review March 7, 2023 08:45
@PicoCentauri PicoCentauri merged commit 65fce73 into main Mar 7, 2023
@PicoCentauri PicoCentauri deleted the fix_ridge_tests branch March 7, 2023 09:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants