Skip to content

Conversation

@rgesteve
Copy link
Contributor

We are excited to review your PR.

So we can do the best job, please check:

  • There's a descriptive title that will make sense to other developers some time from now.
  • [] There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
  • Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
  • You have included any necessary tests in the same PR.

Strictly this is independent of OneDAL (XGBoost and OneDAL are separate libraries), but adding to this feature branch for convenience. This payload includes the PInvoke infrastructure to call XGBoost (version 1.5 and above... current version is 1.7). It mimics the design of the LightGBM integration as much as possible. Most notable difference is that it does not serialize to InternalEnsembleTree, but uses its own model (also it uses the JSON dump to build such a model, but that's an implementation detail). The actual Estimator/Transformer pair is preliminary, as it looks more like a standard transformation than a learner (in that the Model is not an independent entity). This most likely will need to change.

Ubuntu and others added 30 commits October 6, 2021 11:49
Alexsandruss and others added 14 commits September 27, 2022 09:35
* Add DataFrame.IO tests with separators in data

* Add test where comma is in header

* Add two versions of test cases, likely going to use the helper version

* Fix separators in data

* Fix separators in header

* Clean up tests

* Fix issue with not wrapping output with newlines in quotations

* Accidental commit

* Clean up

* Clean up mini test framework a bit

* Apply suggestions from code review

Co-authored-by: Eric Erhardt <[email protected]>

* Apply suggestions from code review

* Apply suggestions from code review

* Write to StreamWriter directly

Co-authored-by: Eric Erhardt <[email protected]>
@LittleLittleCloud
Copy link
Member

Do you want to merge it to onedal-integration or main branch?

@rgesteve
Copy link
Contributor Author

To onedal-integration, please...

@rgesteve rgesteve mentioned this pull request Oct 17, 2022
4 tasks
@rgesteve
Copy link
Contributor Author

Closing in favor of #6383, #6364 and #6373 which include only the relevant files to the changes (rather than the noise introduced by merging from feature branch)

@rgesteve rgesteve closed this Oct 17, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.