Skip to content

Set LDFLAGS and CPPFLAGS for keg-only libomp in CI config#7407

Closed
lagru wants to merge 5 commits intoscikit-image:mainfrom
lagru:patch-clang-macos
Closed

Set LDFLAGS and CPPFLAGS for keg-only libomp in CI config#7407
lagru wants to merge 5 commits intoscikit-image:mainfrom
lagru:patch-clang-macos

Conversation

@lagru
Copy link
Member

@lagru lagru commented Apr 30, 2024

Description

Closes #7406. The output while running "brew install libomp" suggests that the mentioned flags need to be set accordingly. So check whether this addresses the issue.

Checklist

Release note

For maintainers and optionally contributors, please refer to the instructions on how to document this PR for the release notes.

...

@lagru
Set LDFLAGS and CPPFLAGS for keg-only libomp
6343433 
This job started to fail with

 ../meson.build:1:0: ERROR: Compiler /usr/bin/clang cannot compile programs.

The output while running "brew install libomp" suggests that the
mentioned flags need to be set accordingly. So check whether this
addresses the issue.
@lagru lagru added the 🤖 type: Infrastructure CI, packaging, tools and automation label Apr 30, 2024
@lagru
Copy link
Member Author

lagru commented Apr 30, 2024

Great news the compilation works now. However, the tests fail...

=========================== short test summary info ============================
FAILED measure/_moments.py::skimage.measure._moments.moments_hu
FAILED measure/tests/test_fit.py::test_ellipse_parameter_stability - AssertionError: 
Arrays are not almost equal to 7 decimals
 ACTUAL: 2.356194490192345
 DESIRED: 0.7853981633974483
====== 2 failed, 9088 passed, 3 skipped, 12 warnings in 221.30s (0:03:41) ======

@lagru
Copy link
Member Author

lagru commented Apr 30, 2024

@stefanv do you maybe want to take a look at the failing test_ellipse_parameter_stability?

@jarrodmillman jarrodmillman mentioned this pull request May 1, 2024
Merged
@jarrodmillman
Copy link
Contributor

jarrodmillman commented May 2, 2024

Note that macos-latest switched from 12.7.4 to 14.4.1.

@stefanv
Copy link
Member

stefanv commented May 2, 2024

The moments failure is innocuous:

Expected:
    array([0.74537037, 0.35116598, 0.10404918, 0.04064421, 0.00264312,
           0.02408546, 0.        ])
Got:
    array([ 7.45370370e-01,  3.51165981e-01,  1.04049179e-01,  4.06442107e-02,
            2.64312299e-03,  2.40854582e-02, -8.90690725e-21])

@stefanv
Copy link
Member

stefanv commented May 2, 2024 •
edited
Loading

The ellipse failure: the ellipse parameters a and b are correctly estimated (good!). The angle is given as 135 instead of 45. I don't think a pi/2 correction is needed in this case, yet it was added.

        # angle of counterclockwise rotation of major-axis of ellipse
        # to x-axis [eqn. 23] from [2].
        phi = 0.5 * np.arctan((2.0 * b) / (a - c))
        if a > c:
            phi += 0.5 * np.pi

a is definitely larger than c in this case, but what does that comparison do?

The code above is from #2482

I think we should carefully look at https://mathworld.wolfram.com/Ellipse.html#eqn23 too. Question is: why does this pop up now, and only on newer macos / clang compiler? They must have a math library with different trigonometric branching.

The parameter stabilization, just below the above, seems fine:

        # stabilize parameters:
        # sometimes small fluctuations in data can cause
        # height and width to swap
        if width < height:
            width, height = height, width
            phi += np.pi / 2

jarrodmillman added a commit to jarrodmillman/scikit-image that referenced this pull request May 2, 2024
@jarrodmillman
Use macos-12 runner
0ccd7df 
Temporary fix until scikit-image#7407 is resolved
@jarrodmillman jarrodmillman mentioned this pull request May 2, 2024
Merged
@jarrodmillman
Copy link
Contributor

jarrodmillman commented May 2, 2024

As a temporary workaround, we can merge #7408. This PR will need to be rebased and revert that commit. Also note that macos-latest is now macos-14-arm64 (it used to be macos-12):
2024-05-02T10:26:42,749505816-07:00

lagru pushed a commit that referenced this pull request May 6, 2024
@jarrodmillman
Temporarily pin macos-12 runner in CI (#7408)
61f5e01 
Temporary fix until #7407 is resolved
@bsipocz bsipocz mentioned this pull request Oct 13, 2024
Merged
@jarrodmillman jarrodmillman mentioned this pull request Nov 13, 2024
Closed
@lagru
Copy link
Member Author

lagru commented Feb 3, 2025

This doesn't seem to be a problem anymore. Not sure why but still happy to close this. 🤷

@lagru lagru closed this Feb 3, 2025
@lagru lagru deleted the patch-clang-macos branch February 3, 2025 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

🤖 type: Infrastructure CI, packaging, tools and automation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CI fails on MacOS with "clang cannot compile programs"

3 participants

@lagru @jarrodmillman @stefanv