LSSTDESC / rail_base

Base classes for RAIL
MIT License
0 stars 1 forks source link

Adding logic to `CatEstimator` to obey `calculated_point_estimates`. #42

Closed drewoldag closed 11 months ago

drewoldag commented 1 year ago

This PR represents the work to move the majority of the reusable logic for calculating point estimates into the CatEstimator base class. This largely avoids duplicated work in the various estimator subclasses.

When a particular estimator subclass happens to have a particularly efficient way of calculating a given point estimate, the subclass and overwrite the fine grained methods for the particular estimates. And example of this is demonstrated in the unit tests FWIW.

One open question - would it make sense to include a call to _calculate_point_estimates in _do_chunk_output? It seems like all the subclasses make a call to that parent class method in their implementations of _process_chunk. If we included a call to _calculate_point_estimates there, it would save having to make updates to all the estimators.

However, doing so also hides this functionality from the user which probably isn't what we want. I'm open to thoughts here.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.08% :tada:

Comparison is base (03c07ba) 95.48% compared to head (54ed08c) 95.56%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #42 +/- ## ========================================== + Coverage 95.48% 95.56% +0.08% ========================================== Files 31 31 Lines 1595 1625 +30 ========================================== + Hits 1523 1553 +30 Misses 72 72 ``` | [Files Changed](https://app.codecov.io/gh/LSSTDESC/rail_base/pull/42?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LSSTDESC) | Coverage Δ | | |---|---|---| | [src/rail/estimation/estimator.py](https://app.codecov.io/gh/LSSTDESC/rail_base/pull/42?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LSSTDESC#diff-c3JjL3JhaWwvZXN0aW1hdGlvbi9lc3RpbWF0b3IucHk=) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

eacharles commented 1 year ago

Ok, fair enough.

-e

On Aug 16, 2023, at 12:30 PM, Drew Oldag @.***> wrote:

@drewoldag commented on this pull request.

In src/rail/estimation/estimator.py https://github.com/LSSTDESC/rail_base/pull/42#discussion_r1296341700:

  • """Calculates and returns the mean values for a set of posterior estimates
  • in a qp.Ensemble instance.
  • Parameters

  • qp_dist : qp.Ensemble
  • The qp Ensemble instance that contains posterior estimates.
  • Returns

  • NDArray
  • The mean value for each posterior in the qp.Ensemble
  • """
  • return qp_dist.mean()
  • def _calculate_median_point_estimate(self, qp_dist) -> NDArray: The reason this is here is to enable the subclasses to overload this method if the estimator has a very efficient way of producing these values. For a contrived example, if the estimator already calculated and has cached the median, then it could overload this method, and return the cached values.

— Reply to this email directly, view it on GitHub https://github.com/LSSTDESC/rail_base/pull/42#discussion_r1296341700, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADRIGIRH4TAVBPPEML5GNWDXVUNUTANCNFSM6AAAAAA3QKGW2E. You are receiving this because your review was requested.

drewoldag commented 1 year ago

FYI - I will be on vacation the week of Aug 21-25, so I won't be available for comments or commits here. The following week, Aug 28 - Sept 1, I'll be able to catch up on any changes a little, however, most of my time will be committed to working with a visiting group of researchers. The week after that, starting Sept 5, I'll be back on my standard working schedule.