MESMER-X: refactor and test find first guess module #577

veni-vidi-vici-dormivi · 2024-12-06T11:34:19Z

I test the first guess functionality of distrib_cov and along this started refactoring the functions as well.
The refactoring includes:

converting some class attributes (self.var = ...) to locals
making some functions private
use the mean squared error in the loss functions fg_fun_* instead of the summed squared error for easier testing
some renaming and rearranging for better readability

I write tests for find_fg() testing several distributions, a few expressions and a few starting first guesses. Note that the tests generally test the simplest setup possible, like the standard normal or just the mean varying with the predictor. This is to test if we manage to find the right solution for the simplest use cases. As we know, it is impossible to test all expressions and distributions.

Tests added
Fully documented, including CHANGELOG.rst

for more information, see https://pre-commit.ci

codecov · 2024-12-06T11:40:25Z

Codecov Report

Attention: Patch coverage is 98.38710% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.10%. Comparing base (8f75b2e) to head (3fccd28).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
mesmer/mesmer_x/train_l_distrib_mesmerx.py	98.38%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #577      +/-   ##
==========================================
+ Coverage   78.12%   80.10%   +1.98%     
==========================================
  Files          49       49              
  Lines        3012     3066      +54     
==========================================
+ Hits         2353     2456     +103     
+ Misses        659      610      -49

Flag	Coverage Δ
unittests	`80.10% <98.38%> (+1.98%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

veni-vidi-vici-dormivi · 2024-12-06T11:40:32Z

mesmer/mesmer_x/train_l_distrib_mesmerx.py

+            if not globalfit_all.success:
+                raise ValueError(
+                    "Global optimization for first guess failed, please check boundaries_coeff or ",
+                    "disable fg_with_global_opti in options_solver.",
+                )


I'm not sure if this could also only be a warning. Before, if it failed self.coeffs would just be None. @yquilcaille, could that become a problem at some point? I will also look into this a little more.

veni-vidi-vici-dormivi · 2024-12-06T11:44:28Z

tests/unit/test_mesmer_x_first_guess.py

+    )
+    dist2.find_fg()
+    result2 = dist.fg_coeffs
+    np.testing.assert_equal(result2, result)  # No


Providing a first guess does not make the result any better in this case, but might in more difficult cases, or at least speed up the fitting, could also remove this test. Note that the user cannot provide a first guess in xr_train_distrib. We could think about adding this.

Suggested change

np.testing.assert_equal(result2, result) # No

# NOTE: leads to the same result as without first guess

np.testing.assert_equal(result2, result) # No

veni-vidi-vici-dormivi · 2024-12-06T11:46:15Z

tests/unit/test_mesmer_x_first_guess.py

+    # still finds a fg because we do not enforce the bounds on the fg
+    # however the fg is significantly worse on the param with the wrong bounds
+    # in contrast to the above this also runs step 6: fit on CDF or LL^n -> impications?


Not sure what to think about this... might not be a systematic problem....

mathause

I have not looked at the tests yet.

mesmer/mesmer_x/train_l_distrib_mesmerx.py

mathause · 2024-12-20T15:35:38Z

mesmer/mesmer_x/train_l_distrib_mesmerx.py

+        ind_targ_low = np.where(smooth_targ < mean_minus_one_std)[0]
+        ind_targ_high = np.where(smooth_targ > mean_plus_one_std)[0]


use _sm and _lg? (you see I like when words have the same length) but also ok to keep

where? and what would it mean?

sm(all) and l(ar)g(e) ( 🤔 ) and instead of low and high

The fact that I didn't get that maybe speaks again doing that 😅. I see your point, we could also just go with small and large but it's quite long oc. I vote for keep as is.

mesmer/mesmer_x/train_l_distrib_mesmerx.py

veni-vidi-vici-dormivi · 2024-12-20T16:13:31Z

mesmer/mesmer_x/train_l_distrib_mesmerx.py

+        # location might not be used (beta distribution) or set in the expression
+        if len(self.fg_ind_loc) > 0:
+            localfit_loc = self._minimize(
+                func=self._fg_fun_loc,


This is to enable finding a first guess also for expr = Expression("loc=0, scale=c1", expr_name="name". This is a rather major change and it also led me to add two tests to test_mesmer_x_expression and found a bug (#525 (comment)), so this might deserve its own PR?

Yes may be easier to see what is happening if done in a separate PR

mathause

Looking good - thanks. But it's such a large beast so super difficult to have to overview. And also difficult to know what is a limitation of the data and distribution and what one of the code...

mathause · 2025-01-09T07:06:17Z

mesmer/mesmer_x/train_l_distrib_mesmerx.py

@@ -261,6 +257,16 @@ def np_train_distrib(
    return dfit.coefficients_fit, dfit.quality_fit


+def _smooth_data(data, nn=10):


Suggested change

def _smooth_data(data, nn=10):

def _smooth_data(data, length=10):

(or size or something similar)

mathause · 2025-01-09T07:29:08Z

mesmer/mesmer_x/train_l_distrib_mesmerx.py

+        # location might not be used (beta distribution) or set in the expression
+        if len(self.fg_ind_loc) > 0:
+            localfit_loc = self._minimize(
+                func=self._fg_fun_loc,


Yes may be easier to see what is happening if done in a separate PR

mathause · 2025-01-09T07:30:36Z

mesmer/mesmer_x/train_l_distrib_mesmerx.py

-            x0=self.fg_coeffs[self.fg_ind_loc],
-            fact_maxfev_iter=len(self.fg_ind_loc) / self.n_coeffs,
-            option_NelderMead="best_run",
+            [self.expr_fit.coefficients_list.index(c) for c in loc_coeffs]


ok to keep but that would better be done in Expression I think

mathause · 2025-01-09T07:34:18Z

mesmer/mesmer_x/train_l_distrib_mesmerx.py

-            # compared to all 0, better for ref level but worse for trend
-            x0 = np.full(len(scale), fill_value=np.std(self.data_targ))
+        # scale might not be used (beta distribution) or set in the expression
+        if len(self.fg_ind_sca) > 0:


Suggested change

if len(self.fg_ind_sca) > 0:

if len(self.fg_ind_sca) > 0:

mathause · 2025-01-09T07:43:44Z

tests/unit/test_mesmer_x_first_guess.py

+    pred = np.ones(n)
+    targ = rng.normal(loc=0, scale=1, size=n)
+
+    expression = Expression("norm(loc=c1, scale=c3)", expr_name="exp1")


Suggested change

expression = Expression("norm(loc=c1, scale=c3)", expr_name="exp1")

expression = Expression("norm(loc=c1, scale=c2)", expr_name="exp1")

not that is matters...

mathause · 2025-01-09T08:06:25Z

tests/unit/test_mesmer_x_first_guess.py

+    expected = [loc, scale]
+
+    np.testing.assert_allclose(result, expected, rtol=0.1)
+


Also test 1-2 discrete distributions?

mathause · 2025-01-09T08:10:49Z

tests/unit/test_mesmer_x_first_guess.py

+    expected = np.array([-0.005093813, 1.015267311])
+    np.testing.assert_allclose(result, expected, rtol=1e-5)
+
+    # test with wrong bounds


Suggested change

# test with wrong bounds

# test with bounds outside true value

mathause · 2025-01-09T08:12:55Z

tests/unit/test_mesmer_x_first_guess.py

+    )
+    dist.find_fg()
+    result = dist.fg_coeffs
+    expected = np.array([-0.005093817, 1.015267298])


So here we get an estimate outside the bounds, correct?

mathause · 2025-01-09T08:14:30Z

tests/unit/test_mesmer_x_first_guess.py

+    expression = Expression("norm(loc=c1*__tas__, scale=c2)", expr_name="exp1")
+    dist = distrib_cov(targ, {"tas": pred}, expression)
+
+    smooth_targ = dist._smooth_data(targ)


didn't you turn that into a function?

mathause · 2025-01-09T08:20:11Z

tests/unit/test_mesmer_x_first_guess.py

+    dist.fg_ind_loc = np.array([0])
+
+    # test local minima at true coefficients
+    loss_at_toolow = dist._fg_fun_loc(c1 - 1, targ)


Choose a smaller $\Delta$?

veni-vidi-vici-dormivi and others added 2 commits December 6, 2024 12:33

test find_fg

1e90024

[pre-commit.ci] auto fixes from pre-commit.com hooks

bcf2566

for more information, see https://pre-commit.ci

veni-vidi-vici-dormivi commented Dec 6, 2024

View reviewed changes

veni-vidi-vici-dormivi added 26 commits December 6, 2024 12:46

nits

a225cb1

linting

27ccb6b

less digits for expected results

496abc0

more digits

defb257

increase tolerance

bd22628

nits

2a1d6c1

nits

afe51d1

make internal functions private

cc6bb07

fg: refactoring for better readability, comments

63d2c0f

test _fg_func_deriv01

c0970a4

original smoothing function and xfail

6277f6b

add comment for smoothing issue

08beb59

documentation for _fg_fun_deriv01

41df45c

documentation for _fg_fun_loc

4188c90

test for _fg_fun_loc

2570802

change loss from summed squared error to mean squared error

7c3bdd5

documentation of _fg_fun_sca

6cbf3ab

test _fg_fun_loc

9c863c1

nits

e20c9d6

add GEV test case

17fb18d

refactoring of _fg_fun_others

91d5fa9

test robustness for different provided first guesses with normal distrib

bf62b72

test different shape parameters for GEV

5a1c8b9

add test for beta distribution

f42be45

add scale test for GEV

ec1f91a

add documentation to validate_coefficients

492a92f

veni-vidi-vici-dormivi added 8 commits December 18, 2024 14:35

convert provided first guess to floats

95dc00f

refactoring, documentation and TODO for _fg_fun_others

59ab976

find_fg: implement possibility to fix loc and scale

2ed8e39

refactoring

3c9e2e6

add test for _fg_fun_others

acbba6b

more documentation on finf_fg()

50a9489

refactoring

b8dd583

relax rtol for truncnorm

219eca0

veni-vidi-vici-dormivi requested a review from mathause December 20, 2024 13:46

veni-vidi-vici-dormivi changed the title ~~MESMER-X: test find first guess module~~ MESMER-X: refactor and test find first guess module Dec 20, 2024

veni-vidi-vici-dormivi added 2 commits December 20, 2024 15:25

make difference quotient static method

9d3440c

test float in expression

f762b11

mathause reviewed Dec 20, 2024

View reviewed changes

veni-vidi-vici-dormivi commented Jan 5, 2025

View reviewed changes

nits

3fccd28

mathause reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MESMER-X: refactor and test find first guess module #577

MESMER-X: refactor and test find first guess module #577

veni-vidi-vici-dormivi commented Dec 6, 2024 •

edited

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading

veni-vidi-vici-dormivi Dec 6, 2024

veni-vidi-vici-dormivi Dec 6, 2024

mathause Jan 9, 2025

veni-vidi-vici-dormivi Dec 6, 2024 •

edited

Loading

mathause left a comment

mathause Dec 20, 2024

veni-vidi-vici-dormivi Jan 5, 2025

mathause Jan 6, 2025

veni-vidi-vici-dormivi Jan 6, 2025

veni-vidi-vici-dormivi Dec 20, 2024

mathause Jan 9, 2025

mathause left a comment

mathause Jan 9, 2025

mathause Jan 9, 2025

mathause Jan 9, 2025

mathause Jan 9, 2025

mathause Jan 9, 2025

mathause Jan 9, 2025

mathause Jan 9, 2025

mathause Jan 9, 2025

mathause Jan 9, 2025

mathause Jan 9, 2025

	np.testing.assert_equal(result2, result) # No
	# NOTE: leads to the same result as without first guess
	np.testing.assert_equal(result2, result) # No

		ind_targ_low = np.where(smooth_targ < mean_minus_one_std)[0]
		ind_targ_high = np.where(smooth_targ > mean_plus_one_std)[0]

		@@ -261,6 +257,16 @@ def np_train_distrib(
		return dfit.coefficients_fit, dfit.quality_fit


		def _smooth_data(data, nn=10):

	def _smooth_data(data, nn=10):
	def _smooth_data(data, length=10):

	expression = Expression("norm(loc=c1, scale=c3)", expr_name="exp1")
	expression = Expression("norm(loc=c1, scale=c2)", expr_name="exp1")

		expected = [loc, scale]

		np.testing.assert_allclose(result, expected, rtol=0.1)

	# test with wrong bounds
	# test with bounds outside true value

MESMER-X: refactor and test find first guess module #577

Are you sure you want to change the base?

MESMER-X: refactor and test find first guess module #577

Conversation

veni-vidi-vici-dormivi commented Dec 6, 2024 • edited Loading

codecov bot commented Dec 6, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veni-vidi-vici-dormivi Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

mathause left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathause left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veni-vidi-vici-dormivi commented Dec 6, 2024 •

edited

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading

veni-vidi-vici-dormivi Dec 6, 2024 •

edited

Loading