Skip to content
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

pygeosx testing #2981

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

pygeosx testing #2981

wants to merge 4 commits into from

Conversation

bmhan12
Copy link
Contributor

@bmhan12 bmhan12 commented Feb 8, 2024

This PR adds pygeosx unit tests to Github Actions, specfically the gcc ubuntu images.

Related to GEOS-DEV/thirdPartyLibs#259
Related to GEOS-DEV/LvArray#318

@bmhan12 bmhan12 self-assigned this Feb 8, 2024
Copy link

codecov bot commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.25%. Comparing base (6efbd27) to head (fb07769).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2981   +/-   ##
========================================
  Coverage    53.25%   53.25%           
========================================
  Files          989      989           
  Lines        83558    83558           
========================================
  Hits         44495    44495           
  Misses       39063    39063           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bmhan12 bmhan12 force-pushed the feature/han12/pygeosx_docker branch 2 times, most recently from 5abaa1a to 33e7712 Compare February 28, 2024 21:00
@bmhan12 bmhan12 changed the title [WIP] pygeosx testing pygeosx testing Feb 29, 2024
@bmhan12 bmhan12 force-pushed the feature/han12/pygeosx_docker branch from 33e7712 to c72ba06 Compare March 5, 2024 23:23
scripts/ci_build_and_test_in_container.sh Outdated Show resolved Hide resolved

# pygeosx testing
ENABLE_PYGEOSX=${{ inputs.ENABLE_PYGEOSX }}
docker_args+=(-e ENABLE_PYGEOSX=${ENABLE_PYGEOSX:-OFF})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to reduce the number of environment variables and favor direct CLI options.
Do you think we could have the shell script have some kind of --pygeosx flag that would trigger pygeos?
That would make a long list of options indeed, but in the end, it helps us to be more explicit about what we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we could have the shell script have some kind of --pygeosx flag that would trigger pygeos?

Yes, adjusted the script to pass a -DENABLE_PYGEOSX=ON option instead of setting an environment variable.

@bmhan12 bmhan12 force-pushed the feature/han12/pygeosx_docker branch 2 times, most recently from eb9eebc to ae87421 Compare March 14, 2024 15:31
@untereiner
Copy link
Contributor

Should pygeosx be renamed pygeos ? Would it be the good moment to do it ?

@TotoGaz
Copy link
Contributor

TotoGaz commented Mar 14, 2024

Should pygeosx be renamed pygeos ?

Yes it should #2331

Would it be the good moment to do it ?

I guess that would be MAKUTU to steer the change @sframba

Copy link
Contributor

@cssherman cssherman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@bmhan12 bmhan12 force-pushed the feature/han12/pygeosx_docker branch from f892168 to fb07769 Compare April 22, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants