simpeg icon indicating copy to clipboard operation
simpeg copied to clipboard

Implement base objective function classes as abstract ones

Open santisoler opened this issue 1 year ago • 3 comments

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-developers when ready for review.

Reference issue

What does this implement/fix?

Simplify the BaseObjectiveFunction:

  • Decorate __init__, __call__, deriv and deriv2 as abstractmethods.
  • Decorate nP as abstractproperty.
  • 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 fields in their methods or not. Don't rely on every objective function to have has_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:

  • BaseDataMisfit require children to implement __call__, deriv and deriv2.
  • BaseRegularization require children to implement f_m and f_m_deriv

Additional information

santisoler avatar Jan 22 '24 23:01 santisoler

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.

codecov[bot] avatar Jan 22 '24 23:01 codecov[bot]

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 BaseRegularization are required by the BaseSimilarityMeasure?
  • Which ones should be overwritten?
  • BaseSimilarityMeasure should also be an abstract class. What methods and properties should be abstract?

santisoler avatar Feb 29 '24 01:02 santisoler

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.

santisoler avatar Mar 01 '24 18:03 santisoler