Repository: simpeg/simpeg
Pull request #1460 (2024-05-13 20:51:11)

Fix bug on arguments of beta estimator directives
#### Summary
<!-- Add a summary of this Pull Request -->

Fix bug in the constructor of beta estimator directive classes: the `seed` argument was being passed as a positional argument to the parent's constructor, which would assign it to the `n_pw_iter` attribute instead to the `seed`, leading to unwanted behaviours. Pass the arguments to the constructor through keywords to make it more explicit and less prone to errors. Add tests that would fail without this bugfix. Remove the `n_pw_iter` and the `method` arguments from the constructor of `BaseBetaEstimator`.

#### PR Checklist
* [ ] If this is a work in progress PR, set as a Draft PR
* [ ] Linted my code according to the [style guides](https://docs.simpeg.xyz/content/getting_started/contributing/code-style.html).
* [ ] Added [tests](https://docs.simpeg.xyz/content/getting_started/practices.html#testing) to verify changes to the code.
* [ ] Added necessary documentation to any new functions/classes following the
      expect [style](https://docs.simpeg.xyz/content/getting_started/practices.html#documentation).
* [ ] Marked as ready for review (if this is was a draft PR), and converted
      to a Pull Request
* [ ] Tagged ``@simpeg/simpeg-developers`` when ready for review.

#### Reference issue
<!--Example: write "Closes #NNNN" to automatically close that issue on merge.-->

Related to #1290 

#### What does this implement/fix?
<!--Please explain your changes.-->

#### Additional information
<!--Any additional information you think is important.-->


<!--
Once all tests pass and the code has been reviewed and approved, it will be merged into main
-->


santisoler: @dccowan would you like to take a look at this PR? While working on extending the tests for #1392, I noticed that the `seed` argument was not being correctly passed to the parent's constructor, therefore the `seed` for these two directives wasn't being set even if that was the user's desire.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/simpeg/simpeg/pull/1460?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simpeg) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 53.07%. Comparing base [(`c12f51e`)](https://app.codecov.io/gh/simpeg/simpeg/commit/c12f51efe1afb52ad9cf6ffe1e357b85c7a51c0e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simpeg) to head [(`47d2403`)](https://app.codecov.io/gh/simpeg/simpeg/pull/1460?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simpeg).
> Report is 1 commits behind head on main.


<details><summary>Additional details and impacted files</summary>


```diff
@@             Coverage Diff             @@
##             main    #1460       +/-   ##
===========================================
- Coverage   82.54%   53.07%   -29.48%     
===========================================
  Files         168      168               
  Lines       25706    25712        +6     
===========================================
- Hits        21220    13647     -7573     
- Misses       4486    12065     +7579     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/simpeg/simpeg/pull/1460?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simpeg).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simpeg).

santisoler: I'm merging this so we can unblock some other PRs.

Patch:
diff --git a/simpeg/directives/directives.py b/simpeg/directives/directives.py
index 156f1acb3..1fd28d246 100644
--- a/simpeg/directives/directives.py
+++ b/simpeg/directives/directives.py
@@ -355,9 +355,7 @@ class BaseBetaEstimator(InversionDirective):
     def __init__(
         self,
         beta0_ratio=1.0,
-        n_pw_iter=4,
         seed=None,
-        method="power_iteration",
         **kwargs,
     ):
         super().__init__(**kwargs)
@@ -452,7 +450,7 @@ class BetaEstimateMaxDerivative(BaseBetaEstimator):
     """
 
     def __init__(self, beta0_ratio=1.0, seed=None, **kwargs):
-        super().__init__(beta0_ratio, seed, **kwargs)
+        super().__init__(beta0_ratio=beta0_ratio, seed=seed, **kwargs)
 
     def initialize(self):
         if self.seed is not None:
@@ -522,7 +520,7 @@ class BetaEstimate_ByEig(BaseBetaEstimator):
     """
 
     def __init__(self, beta0_ratio=1.0, n_pw_iter=4, seed=None, **kwargs):
-        super().__init__(beta0_ratio, seed, **kwargs)
+        super().__init__(beta0_ratio=beta0_ratio, seed=seed, **kwargs)
         self.n_pw_iter = n_pw_iter
 
     @property


Test patch:
diff --git a/tests/base/test_directives.py b/tests/base/test_directives.py
index 474fba332..aa8348918 100644
--- a/tests/base/test_directives.py
+++ b/tests/base/test_directives.py
@@ -397,5 +397,34 @@ class TestUpdateSensitivityNormalization:
             d_temp.normalization_method = normalization_method
 
 
+class TestBetaEstimatorArguments:
+    """
+    Test if arguments are assigned in beta estimator directives.
+    These tests catch the bug described and fixed in #1460.
+    """
+
+    def test_beta_estimate_by_eig(self):
+        """Test on directives.BetaEstimate_ByEig."""
+        beta0_ratio = 3.0
+        n_pw_iter = 3
+        seed = 42
+        directive = directives.BetaEstimate_ByEig(
+            beta0_ratio=beta0_ratio, n_pw_iter=n_pw_iter, seed=seed
+        )
+        assert directive.beta0_ratio == beta0_ratio
+        assert directive.n_pw_iter == n_pw_iter
+        assert directive.seed == seed
+
+    def test_beta_estimate_max_derivative(self):
+        """Test on directives.BetaEstimateMaxDerivative."""
+        beta0_ratio = 3.0
+        seed = 42
+        directive = directives.BetaEstimateMaxDerivative(
+            beta0_ratio=beta0_ratio, seed=seed
+        )
+        assert directive.beta0_ratio == beta0_ratio
+        assert directive.seed == seed
+
+
 if __name__ == "__main__":
     unittest.main()