Skip to content

add card2pmml method to export a scorecard to pmml#124

Closed
itlubber wants to merge 6 commits intoamphibian-dev:masterfrom
itlubber:master
Closed

add card2pmml method to export a scorecard to pmml#124
itlubber wants to merge 6 commits intoamphibian-dev:masterfrom
itlubber:master

Conversation

@itlubber
Copy link
Copy Markdown

@itlubber itlubber commented May 4, 2023

add card2pmml method to export a scorecard to pmml

@Secbone
Copy link
Copy Markdown
Member

Secbone commented Apr 13, 2026

Thanks for the idea of adding PMML export! This is a useful feature for production deployment scenarios.

However, the current implementation has some issues that need to be addressed before merging:

  1. Undeclared dependencies: sklearn_pandas, sklearn2pmml, and pypmml are not in requirements. sklearn2pmml also requires a Java runtime.
  2. Test quality: Tests use pytest.raises(Exception) then assert e.type != RuntimeError — this passes for any exception including import errors, so it doesn't validate actual PMML output.
  3. Merge conflicts: The PR is 3 years old and scorecard.py has diverged significantly.

Could you consider:

  • Adding the dependencies as optional (pip install toad[pmml])
  • Using pytest.importorskip to skip tests when deps aren't available
  • Rebasing onto latest master
  • Adding a proper integration test that validates the PMML output

Happy to help review a refreshed version!

Copilot AI review requested due to automatic review settings April 14, 2026 02:01
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Secbone added a commit that referenced this pull request Apr 14, 2026
Add ScoreCard.card2pmml() method to export scorecard rules to PMML
format, enabling deployment in Java/PMML ecosystems. This addresses
the feature request from PR #124 with a cleaner implementation.

Changes:
- Add _build_numeric_expression() helper for ExpressionTransformer
- Add card2pmml() method using sklearn2pmml pipeline
- Add requirements-pmml.txt with sklearn2pmml/sklearn-pandas deps
- Add [pmml] optional dependency group in pyproject.toml
- Add comprehensive unit tests and integration tests (with skip guards)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Secbone Secbone self-requested a review April 14, 2026 05:36
Secbone added a commit that referenced this pull request Apr 14, 2026
* feat: add card2pmml() for PMML export of scorecards

Add ScoreCard.card2pmml() method to export scorecard rules to PMML
format, enabling deployment in Java/PMML ecosystems. This addresses
the feature request from PR #124 with a cleaner implementation.

Changes:
- Add _build_numeric_expression() helper for ExpressionTransformer
- Add card2pmml() method using sklearn2pmml pipeline
- Add requirements-pmml.txt with sklearn2pmml/sklearn-pandas deps
- Add [pmml] optional dependency group in pyproject.toml
- Add comprehensive unit tests and integration tests (with skip guards)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: check rules before imports in card2pmml

Move the empty-rules guard before the lazy import block so
test_card2pmml_missing_rules works even when sklearn2pmml
is not installed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
@Secbone Secbone closed this in #160 Apr 14, 2026
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