-
Notifications
You must be signed in to change notification settings - Fork 22
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
add developer API post #198
base: main
Are you sure you want to change the base?
Conversation
Yes that was the goal IIRC. |
Co-authored-by: Jérémie du Boisberranger <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on my side. Now that we released on PyPI, I would fine publishing the blog post and advertise it on the social media to get potential feedbacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if Guillaume's message on discord https://discord.com/channels/731163543038197871/1046822941586898974/1316511187005079553 doesn't receive negative feedback.
cycle warning. | ||
|
||
In the past few releases, we've slowly introduced more functionalities under this | ||
umbrella. `__sklearn_clone__` and `__sklearn_is_fitted__` are two examples. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an easy way to tell what is part of this developer API? If yes we could mention it here. If no, we could create it at some point later (not needed for this blog post)
In the past few releases, we've slowly introduced more functionalities under this | ||
umbrella. `__sklearn_clone__` and `__sklearn_is_fitted__` are two examples. | ||
|
||
In the latest release, at the time of writing this post, we focused on the testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not sure why i cant suggest a change directly)
Should we just say "In the 1.6 release, we focussed on ..."?
``` | ||
|
||
The new tags mostly follow the same structure as the old tags, but there are certain | ||
changes to them. The main change is that the old `_xfail_checks` is no more present |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"is no more present" -> "is no longer present"
``` | ||
|
||
While working on the testing infrastructure, we have also been working on improving our | ||
tests and that means in this release we had a particularly higher number of changes in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests and that means in this release we had a particularly higher number of changes in | |
tests and that means in this release we had a particularly high number of changes in |
|
||
While working on the testing infrastructure, we have also been working on improving our | ||
tests and that means in this release we had a particularly higher number of changes in | ||
their names and what they do. The changes should have made it easier for developers to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The changes should have" -> "The changes will make it easier for"
"should have" somehow sounds like it was a failed attempt. "Increasing prices should have reduced demand for ice cream, however queues are longer than ever"
`check_estimator` and `parametrize_with_checks` to include only strictly API related | ||
tests. | ||
|
||
The above changes means developers need to update their estimators and depending on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above changes means developers need to update their estimators and depending on | |
The above changes mean developers need to update their estimators and depending on |
Left a few style comments, otherwise this looks good to me. Useful information! |
We can also add a link to this from our release highlights.