Skip to content

Refactor: Centralize local model initialization in _BaseModel - #99

Closed
jigyasaba wants to merge 6 commits into
pysal:mainfrom
jigyasaba:refractor
Closed

Refactor: Centralize local model initialization in _BaseModel#99
jigyasaba wants to merge 6 commits into
pysal:mainfrom
jigyasaba:refractor

Conversation

@jigyasaba

Copy link
Copy Markdown
Contributor

Description:

While reviewing the BaseClassifier and BaseRegressor implementations, I noticed the logic for inspecting random_state during local model initialization was duplicated.

I have refactored this into a helper method _initialize_local_model in the parent _BaseModel class. This adheres to the DRY principle and simplifies the _fit_local loop in both subclasses, making the codebase easier to maintain as we add new modules.

Validation:

Verified that random_state is correctly passed to models that support it.

Ran pytest gwlearn/tests/test_base.py and all 112 tests passed.

@codecov

codecov Bot commented Feb 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.09%. Comparing base (34ab4fd) to head (8e34a5c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
+ Coverage   92.96%   93.09%   +0.12%     
==========================================
  Files           6        6              
  Lines         825      825              
==========================================
+ Hits          767      768       +1     
+ Misses         58       57       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@martinfleis

Copy link
Copy Markdown
Member

It is duplicated but explicit. Anyone reading the code immediately gets it without figuring out what does the private method do.

Any PR shall have a linked and approved issue. Closing as this one does not.

@jigyasaba

Copy link
Copy Markdown
Contributor Author

Thank you for the clarification, I understand the preference for explicitness in the fit loops over the DRY refactor here. I also appreciate the reminder on the workflow—I will ensure all future PRs are linked to an approved issue first.

@jigyasaba
jigyasaba deleted the refractor branch February 27, 2026 13:16
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.

2 participants