Skip to content

Return the axis from AalenJohansenFitter.plot_cumulative_density - #1700

Open
VenishPaneliya wants to merge 1 commit into
CamDavidsonPilon:masterfrom
VenishPaneliya:aj-plot-return-and-defaults
Open

VenishPaneliya wants to merge 1 commit into
CamDavidsonPilon:masterfrom
VenishPaneliya:aj-plot-return-and-defaults

Conversation

@VenishPaneliya

Copy link
Copy Markdown

AalenJohansenFitter.plot_cumulative_density returns None

The method documents:

        Returns
        -------
        ax:
            a pyplot axis object

but the body ends with a bare call:

        if not self._calc_var:
            kwargs["ci_show"] = False
        _plot_estimate(self, estimate=self._estimate_name, **kwargs)

Every sibling plot method returns that call — return _plot_estimate(self, estimate=..., **kwargs) — so this is the only one that drops the axis:

>>> type(KaplanMeierFitter().fit(T, E).plot_cumulative_density())
matplotlib.axes._axes.Axes
>>> type(AalenJohansenFitter(calculate_variance=False).fit(T, E, event_of_interest=1).plot_cumulative_density())
NoneType

so ax = ajf.plot_cumulative_density(); ax.set_title(...) raises AttributeError. Added the return.

Regression test: TestAalenJohansenFitter::test_plot_cumulative_density_returns_axis. It fails on master and passes with the fix. I put it in test_estimation.py rather than test_plotting.py deliberately — the TestPlotting class is skipif("DISPLAY" not in os.environ), so a test there wouldn't run in headless CI.

Two documented defaults in CoxTimeVaryingFitter

  • fit(robust=False) was documented as (default: True). Every other fitter documents robust: bool, optional (default=False), so this one was the outlier — and it's a meaningful one, since it told users the Huber sandwich (Wei–Lin) robust standard errors were on by default when they aren't. I matched the house wording.
  • _newton_raphson_for_efron_model(show_progress=False) was documented as (default: True).

I left BreslowFlemingHarringtonFitter.fit's alpha: float, optional (default=0.05) alone even though the signature is alpha=None — that wording is used for alpha in 20 places across the fitters to describe the effective default, so it's a convention rather than a mistake.

Tests

TestAalenJohansenFitter and TestCoxTimeVaryingFitter together: 45 passed with the change against 44 on master — the difference is the new test.

black --line-length 130 --check reports the three touched files as needing reformatting, but it does so identically on an unmodified checkout, so I didn't reformat unrelated code.

The method documents "Returns: ax - a pyplot axis object" but called
_plot_estimate(...) without returning it, so it handed back None. Every
sibling plot method returns _plot_estimate(...), e.g.
KaplanMeierFitter.plot_cumulative_density returns an Axes, so
`ax = ajf.plot_cumulative_density(); ax.set_title(...)` failed only here.
Added the return and a regression test in TestAalenJohansenFitter.

Also corrected two documented defaults in CoxTimeVaryingFitter that
disagree with the code:
- fit(robust=False) was documented as "(default: True)". Every other
  fitter documents robust as "(default=False)"; this one claimed robust
  Huber sandwich errors were on by default when they are not.
- _newton_raphson_for_efron_model(show_progress=False) was documented as
  "(default: True)".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant