-
Notifications
You must be signed in to change notification settings - Fork 12
fix: move extra sort arguments to sorter init #241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the Sorter class hierarchy by moving data dependencies from the sort() method parameters to the class constructor (__init__()). This is an architectural improvement that follows better object-oriented design principles by making sorters stateful objects that are initialized with their required data rather than receiving it on each method call.
Key Changes:
- Moved data parameters (
unit_relationships,contacts,geology_data,structure_data,dtm_data) fromsort()method to__init__()in all Sorter classes - Updated the abstract
sort()method signature to only acceptunitsparameter - Modified
project.pyto pass data during sorter instantiation and implement fallback logic for None values
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| map2loop/sorter.py | Refactored all Sorter classes to accept data in __init__() and access it via instance variables; updated sort() method signatures; added Optional type hints; fixed logic bug in SorterMaximiseContacts |
| map2loop/project.py | Updated sorter instantiation to pass required data; added fallback logic to inject data into sorters when not provided at initialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…op into fix/sorter-clean-arguments
make all requrements a list make all arguments required
…op into fix/sorter-clean-arguments
rabii-chaarani
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks all good to me
Description
📝 Thanks for contributing to map2loop!
Please describe the issue that this pull request addresses and summarize the changes you are implementing.
Include relevant motivation and context, if appropriate.
List any new dependencies that are required for this change.
Fixes #(issue)
Type of change
How Has This Been Tested?
Please describe any tests that you ran to verify your changes.
Provide branch name so we can reproduce.
Checklist:
Checklist continued (if PR includes changes to documentation)