-
Notifications
You must be signed in to change notification settings - Fork 47
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
ENH: add dependency injection point to transform X & y together #167
Open
adriangb
wants to merge
50
commits into
master
Choose a base branch
from
whole-dataset-transformer
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 23 commits
Commits
Show all changes
50 commits
Select commit
Hold shift + click to select a range
1d718ff
ENH: add dependancy injection point to transform X & y together
adriangb 1a6037e
Merge branch 'master' into whole-dataset-transformer
adriangb 28535b2
Merge branch 'master' into whole-dataset-transformer
adriangb c170f4b
Extend data transformer notebook with examples of data_transformer usage
adriangb cd5f415
Merge branch 'whole-dataset-transformer' of https://github.com/adrian…
adriangb b7fb34c
run entire notebook
adriangb d3357e4
Merge branch 'master' into whole-dataset-transformer
adriangb bc92cff
Update docstring
adriangb 45887e4
Merge branch 'whole-dataset-transformer' of https://github.com/adrian…
adriangb 5b8e133
typo
adriangb 2f2b7a5
Merge branch 'master' into whole-dataset-transformer
adriangb 6ee6425
Test pipeline, move notebook to markdown
adriangb a3092c2
fix undef transformer
adriangb 8f92591
remove unused dummy transformer
adriangb fa728c1
Remove unused import
adriangb 6fdea0d
remove empty cell
adriangb 8aba7cb
Merge branch 'master' into whole-dataset-transformer
adriangb 6675889
Fix typos
adriangb c317699
Merge branch 'whole-dataset-transformer' of https://github.com/adrian…
adriangb 5acbd0f
add comment
adriangb 5d9e02b
print all data
adriangb 9b43e9c
Update data transformer docs
adriangb 0fbecd0
Merge branch 'master' into whole-dataset-transformer
adriangb deb4858
Finish sentence
adriangb 981e61c
PR feedback
adriangb 0d55306
fix error
adriangb 3cf1ed5
use embedded links, ref links seem to be broken
adriangb a198eb3
spacing
adriangb 047d430
fix code block
adriangb 5413015
Merge branch 'master' into whole-dataset-transformer
adriangb e71625e
Merge branch 'master' into whole-dataset-transformer
adriangb f4c0dcc
Merge branch 'master' into whole-dataset-transformer
adriangb 54cfc43
PR feedback
adriangb 1742ef4
Merge branch 'whole-dataset-transformer' of https://github.com/adrian…
adriangb d03248f
use code block for signature
adriangb 87452ff
remove dummy parameter
adriangb d2b4402
Merge branch 'master' into whole-dataset-transformer
adriangb 034fc7f
Merge branch 'master' into whole-dataset-transformer
adriangb 491e0b1
Merge branch 'master' into whole-dataset-transformer
adriangb 3f8f9b4
re-add dummy
adriangb f569b48
Merge branch 'master' into whole-dataset-transformer
adriangb 8dafe1b
Merge branch 'whole-dataset-transformer' of https://github.com/adrian…
adriangb f918966
Merge master
adriangb fd62b82
Use dicts, add more examples
adriangb 6eee3c4
fix broken test
adriangb 5ca7da8
update docs
adriangb 5bd222e
add clarifying comment in docs
adriangb f560687
update TOC
adriangb 2353408
Merge branch 'master' into whole-dataset-transformer
adriangb f5df4c4
Merge branch 'master' into whole-dataset-transformer
adriangb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think this diagram is really useful.
Would this be a better diagram?
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.
I do like the addition of
.fit
. My only worry is that users might think that SciKeras always creates atf.data.Dataset
, which is not the case, by default it gives numpy arrays toKeras.fit
. Do you thinkKeras.fit(dataset or np.array)
makes that clear? It could also be dicts of lists or something, but that's at least more uncommon.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.
What can
dataset_transformer
return? Only datasets/ndarrays? Or does it support the other arguments of Keras.fit?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.
Anything that
Keras.fit
will accept. Internally, it looks something like this: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.
Maybe
Keras.fit(data)
is a good way to specify this? That way there's not confusion in interpretingdataset
astf.data.Dataset
. I can also add a small code block like in #167 (comment) if that helps explain it.