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

Implement batch_size=-1 #194

Merged
merged 7 commits into from
Feb 26, 2021
Merged

Implement batch_size=-1 #194

merged 7 commits into from
Feb 26, 2021

Conversation

adriangb
Copy link
Owner

No description provided.

@github-actions
Copy link

github-actions bot commented Feb 15, 2021

📝 Docs preview for commit 2c65c50 at: https://www.adriangb.com/scikeras/refs/pull/194/merge/

@codecov-io
Copy link

codecov-io commented Feb 15, 2021

Codecov Report

Merging #194 (2c65c50) into master (d83bffa) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #194   +/-   ##
=======================================
  Coverage   99.70%   99.71%           
=======================================
  Files           6        6           
  Lines         679      693   +14     
=======================================
+ Hits          677      691   +14     
  Misses          2        2           
Impacted Files Coverage Δ
scikeras/wrappers.py 99.45% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d83bffa...2c65c50. Read the comment docs.

@stsievert
Copy link
Collaborator

I'm still a fan of keeping **kwargs in model.fit as expressed in #159 (comment). This PR is tangential to that.

Style nit: Skorch batch_size=-1 for the same functionality this PR implements (Skorch docstring). I think that's a little better because it mirrors the negative list indexing.

@adriangb
Copy link
Owner Author

I'm still a fan of keeping **kwargs in model.fit as expressed in #159 (comment). This PR is tangential to that.

Agreed, I don't plan on removing **kwargs just because this is implemented (if it is). Also tangential, but I'm going to punt a final decision on **kwargs down the road since I think some more use cases / user input would be good before making a decision either way.

-1 should work as well, I do like that it avoids introducing a new type to the parameter (str). Thanks for pointing this out.

@adriangb adriangb marked this pull request as ready for review February 15, 2021 17:51
@adriangb adriangb changed the title Implement batch_size=all Implement batch_size=-1 Feb 15, 2021
@adriangb
Copy link
Owner Author

I think this change is pretty non-controversial, especially since Skorch already implements the exact same feature.

I'm going to leave this open a couple more days for feedback and then if there are no objections I'll go ahead and merge it.

@adriangb adriangb merged commit d1a9d12 into master Feb 26, 2021
@adriangb adriangb deleted the batch-size-all branch February 26, 2021 02:09
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.

3 participants