simpeg
simpeg copied to clipboard
Implement base objective function classes as abstract ones
Summary
Draft for redesigning base classes for objective functions, data misfits and regularizations as abstract classes.
PR Checklist
- [ ] If this is a work in progress PR, set as a Draft PR
- [ ] Linted my code according to the style guides.
- [ ] Added tests to verify changes to the code.
- [ ] Added necessary documentation to any new functions/classes following the expect style.
- [ ] Marked as ready for review (if this is was a draft PR), and converted to a Pull Request
- [ ] Tagged
@simpeg/simpeg-developerswhen ready for review.
Reference issue
What does this implement/fix?
Simplify the BaseObjectiveFunction:
- Decorate
__init__,__call__,derivandderiv2asabstractmethods. - Decorate
nPasabstractproperty. - Remove the
mapping. Only regularizations needs it. Data misfits rely on the simulation to handle mapping operations.
Simplify the ComboObjectiveFunction:
- Remove the
W, it was overwritten by child classes, and the implementation only applied when the objective functions used and L2 norm. - Simplify how it checks if the child objective functions need to get
fieldsin their methods or not. Don't rely on every objective function to havehas_fields(it doesn't make sense for regularization to have this attribute).
Remove L2ObjectiveFunction. It was only use to create the BaseDataMisfit
class, forcing it to be only an L2 class. Moreover, most of its methods were
being overwritten by the BaseDataMisfit and L2DataMisfit, so it didn't have
a clear purpose.
Make BaseDataMisfit and BaseRegularization abstract classes:
BaseDataMisfitrequire children to implement__call__,derivandderiv2.BaseRegularizationrequire children to implementf_mandf_m_deriv
Additional information
Codecov Report
Attention: Patch coverage is 82.50000% with 14 lines in your changes are missing coverage. Please review.
Project coverage is 82.48%. Comparing base (
aa0a258) to head (625bf58). Report is 1 commits behind head on main.
| Files | Patch % | Lines |
|---|---|---|
| simpeg/objective_function.py | 85.96% | 8 Missing :warning: |
| simpeg/data_misfit.py | 72.22% | 5 Missing :warning: |
| simpeg/regularization/base.py | 80.00% | 1 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #1335 +/- ##
==========================================
- Coverage 82.54% 82.48% -0.07%
==========================================
Files 168 168
Lines 25706 25643 -63
==========================================
- Hits 21220 21151 -69
- Misses 4486 4492 +6
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I noticed by running some tests on the drafted code that some regularization classes like JointTotalVariation, CrossGradient and LinearCorrespondence don't implement the f_m and/or the f_m_deriv methods. They are all childs of BaseSimilarityMeasure which overrides some methods of the BaseRegularization class.
I think we should revisit the design of these classes so we avoid violating Liskov substitution principle.
- What methods of the
BaseRegularizationare required by theBaseSimilarityMeasure? - Which ones should be overwritten?
BaseSimilarityMeasureshould also be an abstract class. What methods and properties should be abstract?
After discussing with @lheagy, we decided not to touch the BaseRegularization class in this PR. That branch of the hierarchy tree deserves some more thoughts and trying to tackle it here would create a massive PR. Lets focus on the BaseObjectiveFunction and in the DataMisfit branch of the tree.