Skip to content

Cherry-pick bug fixes and samples from master to release/1.0 #3303

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

Closed
wants to merge 16 commits into from

Conversation

codemzs
Copy link
Member

@codemzs codemzs commented Apr 11, 2019

Cherry pick for the below PRs from master branch, most of these PRs are related to samples and none related to API except for #3172 multi-column mapping API for normalizer estimator and #3291 that makes Prior trainer accept only Boolean type label column to make it consistent with every other binary trainer. I also did speak with @shauheen prior to cherry-picking and he agreed ideally these sample fixes should be in the release.

#3172
#3215
#3212
#3230
#3237
#3257
#3259
#3249
#3267
#3278
#3213
#3285
#3287
#3291
#3295

codemzs added 16 commits April 11, 2019 16:22
* Multi-column mapping for Normalizer estimators.

* XML comment.

* revert Program.cs

* Add copyright header.

* Add tests.

* PR feedback.

* cleanup.

* PR feedback.
* Fix bug in ONNX scorer sample.

* PR feedback.
* Fix SDCA sample runtime exception.

* PR feedback.

* More fixes.
* Fix runtime exception in ImageClassification.

* Cleanup.

* revert changes.

* 3P license notice.
* Add time series samples for stateful prediction engine.

* PR feedback.

* PR feedback.

* PR feedback.

* cleanup.

* PR feedback.

* cleanup.
@shmoradims
Copy link

@codemzs please don't include code changes and documentation work in the same PR, if you want more people to be able to approve it. Now I can't approve this and you need to wait for Shauheen to approve.

@codemzs
Copy link
Member Author

codemzs commented Apr 12, 2019

@shmoradims which commit are you referring to as documentation work here?

@shmoradims
Copy link

shmoradims commented Apr 12, 2019

I'm approving all the changes related to API samples and xml documentation changes.

@shauheen please review changes in SimpleTrainer.cs as I didn't know if you want to take it or not.

Copy link

@shmoradims shmoradims left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

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

So, let me point something out.

You have this (partial) commit history as part of this PR.

image

That first commit introduces an error that would make the project unable to build, until you fixed the error in the last commit. So if we accepted this as written, we'd have in our release/1.0 branch seven commits that did not build, thanks to this one cherry pick. Please rectify.

I am also unsure why a commit that describes itself as changing a namespace is introducing an entirely new sample. My best guess is that you made the commit for a sample that has not yet been cherry picked. That's potentially going to cause problems for whoever chooses cherry pick that sample later, dirties the commit history, etc.

Either way, it's something to be fixed.

Thanks!

{
class MapKeyToVector
{
/// This example demonstrates the use of MapKeyToVector by mapping keys to floats[].
Copy link
Contributor

@TomFinley TomFinley Apr 12, 2019

Choose a reason for hiding this comment

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

Ah... here's another one I found. This cherry pick (commit id eb57d09) supposedly corresponds to #3287, but it includes content that was introduced originally in #3211, which should have been cherry picked separately. @shauheen for comment and direction on this.

Edit: I see there's already a cherry-pick PR for this in #3264 from @sfilipi. Happily that one has already been approved. Might be best for that to go in first.

@TomFinley
Copy link
Contributor

I am also unsure why a commit that describes itself as changing a namespace is introducing an entirely new sample. My best guess is that you made the commit for a sample that has not yet been cherry picked. That's potentially going to cause problems for whoever chooses cherry pick that sample later, dirties the commit history, etc.

OK, so my guess was right. 😄 This sample, which you introduce as part of your commit 6593ed5 (supposedly corresponding to #3267), was actually introduced in #3099. Happily @zeahmed has a cherry pick PR #3240. If you want to structure things in this fashion, it might be best if you had that cherry pick go in before yours.

@shauheen
Copy link
Contributor

All done as part of #3333 . Thanks @codemzs .

@shauheen shauheen closed this Apr 14, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants