Skip to content

fix: add y param and fix n_features_in_ for sklearn compatibility#3364

Open
Famous077 wants to merge 4 commits into
pgmpy:devfrom
Famous077:fix/sklearn-estimator-checks
Open

fix: add y param and fix n_features_in_ for sklearn compatibility#3364
Famous077 wants to merge 4 commits into
pgmpy:devfrom
Famous077:fix/sklearn-estimator-checks

Conversation

@Famous077

Copy link
Copy Markdown

The following checklist is mandatory

Your PR will be closed if you remove the checklist. Use of LLMs is strictly forbidden for any part of this checklist (including for improving language), and will result in a ban if we find any use of LLMs.

Your checklist for this pull request

  • Have you followed all the steps from our Contributing Guide?
  • Does the PR fully address the linked issue and is within its defined scope? If you are still working on the PR, mark it as draft.
  • Are all the GitHub Actions checks passing? If not, mark your PR as draft while you fix it.

If you have used AI/LLMs for any assistance, please answer the following questions. Please refer #2622 for an example of the level of detail we expect:

  • Have you reviewed our AI usage policy?

  • Are you able to fully explain your changes? We expect you to fully understand the algorithm and take full responsibility for any changes in this PR.

  • Please describe in detail how and what you used AI assistance for? Please outline your whole workflow with the AI tool.
    I'm beginner in this org. So, I most used AI for understand the codebase easily with simple analogy, and also for understand issue. what type of issue is this , what are the problems that are occuring due to this issue.

  • What steps have you taken to verify that the changes correctly address the issue? What edge cases have you considered? Other than running tests, what else have you verified?

I used pytest module to verify that the changes correctly address the issue. I haven't faced any edge cases situation. Haven't verify anything else than running tests.

  • Have you used AI for generating tests? Can you compress them into a smaller number of tests without losing coverage?

No, I didn't generate any AI test. I used pytest module to verify after changes everything is working correctly or not.

Issue number(s) that this pull request fixes

List of changes to the codebase in this pull request

  • Added a y=None parameter and self.n_features_in_ = X.shape[1] in causal_discovery/_base.py.

@Famous077 Famous077 marked this pull request as draft April 30, 2026 08:03

@DARHWOLF DARHWOLF left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • Please refer to my comment regarding the use of comments.
  • may I request you to share how you debugged this issue.

Comment thread pgmpy/tests/test_causal_discovery/test_HillClimbSearch.py Outdated
Comment thread pgmpy/tests/test_causal_discovery/test_PC.py Outdated
Comment thread pgmpy/tests/test_causal_discovery/test_ExpertInLoop.py Outdated
@Famous077

Copy link
Copy Markdown
Author

Hi @DARHWOLF , In this issue, Two specific sklearn checks were failing, In supervised learning, y label which is compare against predictions which was missing on score() function, due to which we are getting TypeError. another one is n_feature_in_ is set properly in def fit() function, but missing in def score() one. So, I add same line in def score() to prevent mismatches later. Because this functions tells sklearn how many features the model was trained on. lastly why I commented those line, these 2 checks failure are expected ones. So, whenever these checks get failed rather than produce warning those commented line were meant to be sent. So, I was verifying without those everything works perfectly or not, and as a result everything was working perfectly and all tests were passed. Love to take feedback, if I made any mistakes.

@Famous077 Famous077 requested a review from DARHWOLF May 2, 2026 17:49
@Famous077

Famous077 commented May 19, 2026

Copy link
Copy Markdown
Author

Hi @ankurankan @DARHWOLF , Could you please review this PR whenever you have time.

@Famous077 Famous077 marked this pull request as ready for review May 19, 2026 18:58
@github-actions

Copy link
Copy Markdown
Contributor

This PR has had no activity for 21 days and is now marked as stale.

@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.85%. Comparing base (72b58ae) to head (f24d851).
⚠️ Report is 12 commits behind head on dev.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #3364   +/-   ##
=======================================
  Coverage   95.84%   95.85%           
=======================================
  Files         578      578           
  Lines       32652    32653    +1     
=======================================
+ Hits        31295    31299    +4     
+ Misses       1357     1354    -3     
Files with missing lines Coverage Δ
pgmpy/causal_discovery/_base.py 98.04% <100.00%> (+<0.01%) ⬆️
...y/tests/test_causal_discovery/test_ExpertInLoop.py 96.13% <100.00%> (ø)
pgmpy/tests/test_causal_discovery/test_GES.py 98.91% <100.00%> (ø)
...ests/test_causal_discovery/test_HillClimbSearch.py 99.38% <100.00%> (ø)
pgmpy/tests/test_causal_discovery/test_PC.py 97.32% <100.00%> (ø)

... and 1 file with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] sklearn check_n_features_in_after_fitting fail for causal discovery estimators

2 participants