-
Notifications
You must be signed in to change notification settings - Fork 217
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
Enhance cross-platform compatibility for loading PySRRegressor models #681
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 10138721258Details
💛 - Coveralls |
Thanks, this is great! I'll do a code review for suggesting some changes |
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.
Thanks again, added some comments. Also could you add some unittests for CustomUnpickler
? The test should be one that is currently failing, so this CustomUnpickler
is required to get that unittest working.
(By the way, feel free to click "Resolved" on any of the comments once they are completed) |
Got it :) |
Looking good! Thanks. I think it just needs some tests now – ideally tests that fail with the current master branch, but are fixed by this PR |
One option is to add a test that is only run on the Windows tests – it could generate and then pickle the model on Windows, and then create and run a dockerfile that attempts to unpickle the model? (Since docker runs linux). |
Thank you for the feedback. I agree tests would be valuable, but I'm not experienced in writing test cases. Could you provide some guidance or examples for appropriate tests for these changes? |
I appreciate your suggestion, but I'm afraid I'm a bit lost on how to implement this. The process of generating, pickling, and then using Docker for unpickling sounds complex. I'm not sure where to start. Would you be able to help implement this test, or provide a more detailed guide? |
Ok what I would do is:
I think this should work. It might be overcomplicated though, maybe there's an easier way to test this. Feel free to take a different approach to test your new addition. For simple pure-Python tests you can just add a new test case to the class in Lines 285 to 297 in 3aee19e
|
This PR improves the cross-platform compatibility of the
PySRRegressor.from_file
method, allowing seamless loading of models saved on different operating systems (Windows and Linux).Key changes:
CustomUnpickler
) to handle path objects from different operating systems.path_to_str
function to convert path objects to strings, ensuring consistent path handling across platforms.These changes allow users to load PySRRegressor models on any supported platform, regardless of where the model was originally saved. This enhancement is particularly useful for users working in mixed environments or collaborating across different operating systems.
The modifications are contained within the
from_file
method to minimize impact on other parts of the codebase. The method remains backwards compatible with existing usage patterns.Testing:
This enhancement addresses the issue of cross-platform incompatibility when loading saved models, improving the overall user experience and flexibility of PySRRegressor.