Open zzccchen opened 4 months ago
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
---|---|---|---|---|---|
pysr/utils.py | 12 | 14 | 85.71% | ||
<!-- | Total: | 15 | 17 | 88.24% | --> |
Files with Coverage Reduction | New Missed Lines | % | ||
---|---|---|---|---|
pysr/export_sympy.py | 3 | 77.27% | ||
<!-- | Total: | 3 | --> |
Totals | |
---|---|
Change from base Build 10137135457: | -0.3% |
Covered Lines: | 1141 |
Relevant Lines: | 1221 |
Thanks, this is great! I'll do a code review for suggesting some changes
(By the way, feel free to click "Resolved" on any of the comments once they are completed)
(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).
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
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?
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).
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:
pysr/test/test_pickle.py
pysr/test/test_pickle.dockerfile
test_pickle.py
to run a simple PySR example where the equation file parameter set to some Path
to a folder created with https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory. Also change the class name to TestPickle
or something.TestPickle
.test_dev
throughout the codebase, and use that to figure out where you need to add the equivalent test_pickle
.test_pickle.dockerfile
, specifically all of these lines: https://github.com/MilesCranmer/PySR/blob/3aee19e38ceb3e0e1617d357a831400e01204658/pysr/test/test_dev_pysr.dockerfile#L34-L55 to try to load the pickle file generated by the test you created in (3). And basically just verify that the equations loaded are correct.test_pickle.py
, but reference the test_pickle.dockerfile
. You should be able to pass the filename as an argument.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 test.py
, such as https://github.com/MilesCranmer/PySR/blob/3aee19e38ceb3e0e1617d357a831400e01204658/pysr/test/test.py#L285-L297
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.