Skip to content

Conversation

@leostimpfle
Copy link
Collaborator

@leostimpfle leostimpfle commented Jan 6, 2026

Few adjustments to avoid FutureWarning: ChainedAssignmentError and SettingWithCopyWarning. Closes #1122

@codecov
Copy link

codecov bot commented Jan 6, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pyfixest/estimation/fepois_.py 80.00% 1 Missing ⚠️
Flag Coverage Δ
core-tests 75.08% <80.00%> (?)
tests-extended ?
tests-vs-r 17.56% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pyfixest/estimation/fepois_.py 90.66% <80.00%> (+34.22%) ⬆️

... and 44 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@leostimpfle leostimpfle marked this pull request as ready for review January 6, 2026 10:14
@leostimpfle leostimpfle marked this pull request as draft January 6, 2026 10:15
@leostimpfle leostimpfle marked this pull request as ready for review January 6, 2026 12:20
@leostimpfle leostimpfle requested a review from s3alfisc January 6, 2026 12:21
@leostimpfle
Copy link
Collaborator Author

@s3alfisc Can we merge this small fix? 🙂

self._X.drop(na_separation, axis=0, inplace=True)
self._fe.drop(na_separation, axis=0, inplace=True)
self._data.drop(na_separation, axis=0, inplace=True)
self._Y = self._Y.drop(na_separation, axis=0, inplace=False)
Copy link
Member

Choose a reason for hiding this comment

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

Why can't we do this in place? Would it not be better to just add an ignore flag? Not doing the operation in place is likely less efficient?

Copy link
Member

Choose a reason for hiding this comment

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

Approved, only minor comment was this ⬆️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The SettingWithCopyWarning indicates that the attributes are not properly set upstream. You're right that we should keep it inplace and fix the root cause (probably where they assigned for the first time). Will investigate

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.

Clean up tests to avoid FutureWarning: ChainedAssignmentError

3 participants