Conversation
lucascolley
left a comment
lucascolley
left a comment
There was a problem hiding this comment.
@steppi here are all the changes needed to get macOS tests running in CI. Of course, the tolerances need adjustments.
As a separate bit of fun, CI is now exactly reproducing the errors I saw on my linux box.
Co-authored-by: Albert Steppi <1953382+steppi@users.noreply.github.com>
|
The default compiler for macOS from conda-forge looks to be |
|
okay, I see locally, let's see if CI agrees |
| std::string get_platform_str() { | ||
| /* This is going to get a string "<compiler>-<os>-<architecture>" using conditional | ||
| * compilation, but for now we're just stubbing things out. */ | ||
| return "gcc-linux-x86_64"; | ||
| } |
There was a problem hiding this comment.
I added the tolerance files for Mac, but we need to use preprocessor directives here to make sure the correct file gets selected for each platform. I was looking at this, https://stackoverflow.com/questions/152016/detecting-cpu-architecture-compile-time.
There was a problem hiding this comment.
Okay, cool.
Long-term, we could look at specifying the compiler version here as well? Thinking ahead, having a shared Pixi workspace between xsf and xsfref should allow the reference values to be generated by the same envs as they are tested in against the implementations.
There was a problem hiding this comment.
Yeah. I originally did specify the compiler version but was trying to avoid having too many files. Long term I think we should be using some kind of cloud storage solution instead of stashing the parquet files on Github, and then it should be fine to get very granular with the platforms/compilers/whatever in the tolerance files. For now, maybe it's fine to just allow individual adjustments of the wiggle factor in adjust_tolerance in tests.
tests/testing_utils.h
Outdated
| template <typename T> | ||
| T adjust_tolerance(T tol) { | ||
| // Add some wiggle room to tolerance from table. | ||
| return 4 * std::max(std::numeric_limits<T>::epsilon(), tol); |
There was a problem hiding this comment.
For failures due to different compiler versions, like the currently failing Linux tests, I think it's fine just to increase the wiggle room factor here for now, as long as it doesn't get too large.
|
The 3 Mac OS failures look real. I think SciPy uses a wrapper for those functions which handles the |
|
I just yanked those failing tests out. |
steppi
left a comment
steppi
left a comment
There was a problem hiding this comment.
CI is passing now. Thanks @lucascolley! I'm going to go ahead and merge this so we can continue iterating from here.
2 participants
TODO: