Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added conditional property to expose time scale predictions #327

Closed
wants to merge 12 commits into from
Closed

Added conditional property to expose time scale predictions #327

wants to merge 12 commits into from

Conversation

Finesim97
Copy link
Contributor

@Finesim97 Finesim97 commented Dec 9, 2022

Checklist

Added a decorator for properties, which are only available, if a check returns true. The decorator provided by scikit-learn only works for functions sadly.

@sebp
I am not sure what to test exactly, maybe a test which tests whether pipelines correctly patch the property and functions through? I also think this should not show up in the documentation, as it is internal?

@Finesim97
Copy link
Contributor Author

Fixed the Codacy issue.

@sebp
Copy link
Owner

sebp commented Dec 18, 2022

@Finesim97 Could you please rebase upon latest master, which fixes a couple of issues with CI.

@Finesim97
Copy link
Contributor Author

Happy new year to you! Rebased it.

@codecov
Copy link

codecov bot commented Jan 2, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (d5fcbc9) 97.92% compared to head (445454c) 97.95%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #327      +/-   ##
==========================================
+ Coverage   97.92%   97.95%   +0.03%     
==========================================
  Files          37       37              
  Lines        3323     3379      +56     
  Branches      502      510       +8     
==========================================
+ Hits         3254     3310      +56     
  Misses         33       33              
  Partials       36       36              
Impacted Files Coverage Δ
sksurv/__init__.py 100.00% <100.00%> (ø)
sksurv/util.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Finesim97
Copy link
Contributor Author

Imports wrongly sorted, will fix it

@sebp
Copy link
Owner

sebp commented Feb 26, 2023

@Finesim97 Do you plan to continue working on this?

@Finesim97
Copy link
Contributor Author

Hi,
The server I used to work on this went up in flames and I started my new job, but I will fix this over the following days!

@Finesim97
Copy link
Contributor Author

Sry, I ran the wrong command and screwed up my commits, fixing it at the moment

@Finesim97
Copy link
Contributor Author

Because it is implemented with a @property, the mechanism from scikit-learn did not work (it only supports functions), instead I added a @Property with support for conditional checks.

@Finesim97
Copy link
Contributor Author

I will add some tests to fix codecov

@Finesim97
Copy link
Contributor Author

Hi, I added a test for the conditional property.

@Finesim97
Copy link
Contributor Author

Sorry that everything took so long, but I think now the PR should be ready!

Copy link
Owner

@sebp sebp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your effort. Looks good, except for some small changes.


def _run_check(self, obj):
attr_err = AttributeError(
f"This {repr(obj)} has no attribute {repr(self._name)}"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of {repr(obj)}, you can use {obj!r}, and move the definition of attr_err into the if block.

@@ -260,3 +260,83 @@ def safe_concat(objs, *args, **kwargs):
concatenated[name] = pd.Categorical(concatenated[name], **params)

return concatenated


class _ConditionalAvailableProperty:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if you could inherit from the built-in property, because it is a class. This could allow removing the setter/getter/deleter code.

@sebp sebp mentioned this pull request Jun 10, 2023
5 tasks
@sebp
Copy link
Owner

sebp commented Jun 10, 2023

@Finesim97 Thanks for your contributions. PR #374 is based on your work. This PR is now obsolete.

@sebp sebp closed this Jun 10, 2023
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.

SciKit-Learn Pipeline not patched with "_predict_risk_score"
2 participants