Refactor the convergence logic #373
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -30,7 +30,7 @@ end | |||
| initialization::Symbol = :add_perturbation, | |||
| generative_model_params::NamedTuple = (;), | |||
| min_success_rate::AbstractFloat=0.99, | |||
| converge_when::Symbol=:decision_threshold, | |||
| convergence::Union{AbstractConvergence,Symbol}=:decision_threshold, | |||
| invalidation_rate::AbstractFloat=0.5, | |||
There was a problem hiding this comment.
Redundancies such as the unneccessary keyword arguments here will be removed as part of another PR.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #373 +/- ##
==========================================
+ Coverage 77.04% 77.71% +0.66%
==========================================
Files 69 74 +5
Lines 1577 1557 -20
==========================================
- Hits 1215 1210 -5
+ Misses 362 347 -15 ☔ View full report in Codecov by Sentry. |
| convergence::Union{AbstractConvergence,Symbol}=Convergence.DecisionThresholdConvergence(; | ||
| decision_threshold=(1 / length(data.y_levels)) | ||
| ), |
There was a problem hiding this comment.
Doesn't it always overwrites the convergence value?
There was a problem hiding this comment.
I don't think so - it's the same kind of default argument as the ones other keyword arguments here are using. If the user provides a different convergence value, that will be used instead. The tests should also fail if this is always used as the convergence value. Am I missing something here?
src/convergence/invalidation_rate.jl
Outdated
There was a problem hiding this comment.
As mentioned in #371, I don't think this should be handled as it's own convergence class, but rather under generator conditions. Happy to park that for later though, since we want to move on with this.
| convergence::Union{AbstractConvergence,Symbol}=Convergence.DecisionThresholdConvergence(; | ||
| decision_threshold=(1 / length(data.y_levels)) | ||
| ), |
This pull request is part of the refactor to simplify the core struct and related methods. In it, I propose the following changes:
converged()method. This removes a ton of kwargs from the main struct and makes the code clearer in general. For the more casual users, nothing changes after this refactor: they can still use the:converge_whenkeyword argument to specify a convergence criterion with the default arguments. The more advanced user will need to be a bit more involved than before to define custom convergence criteria: they will have to define one of the new convergence structs on their own and pass that to thegenerate_counterfactual()method. However, the improved clarity of the code seems to justify the added hassle for those users.converged()method that had been created specifically for Growing Spheres. There's no reason Growing Spheres shouldn't fit into the pre-existing convergence framework.:early_stoppingconvergence type, which appears to be functionally the same as the:max_iterconvergence type.