Skip to content

Conversation

@xgarrido
Copy link
Collaborator

pixell is broken with the latest version of pillow (see #65 (comment))

pixell is broken with the latest version of pillow (see #65 (comment))
@xgarrido xgarrido added the fix label Jul 17, 2023
@xgarrido xgarrido changed the title Do not test notebook compilation Fixing API change in pixell 0.19.0 Jul 17, 2023
@xzackli
Copy link
Contributor

xzackli commented Jul 18, 2023

@xgarrido should we bound pixell versions to < 0.18?

@xgarrido
Copy link
Collaborator Author

Actually this branch is for trying to use pixell latest version 0.19.0 and thus fix the unit tests. There is a small change in the pixell API but the tests remains broken.

@xgarrido
Copy link
Collaborator Author

@xzackli in the current master branch we now downgrade pixell to fix the unit test. We will work on new pixell version later

thibautlouis and others added 11 commits September 15, 2023 13:45
Bumps [pypa/cibuildwheel](https://github.com/pypa/cibuildwheel) from 2.14.1 to 2.15.0.
- [Release notes](https://github.com/pypa/cibuildwheel/releases)
- [Changelog](https://github.com/pypa/cibuildwheel/blob/main/docs/changelog.md)
- [Commits](pypa/cibuildwheel@v2.14.1...v2.15.0)

---
updated-dependencies:
- dependency-name: pypa/cibuildwheel
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2023

Codecov Report

Merging #69 (398bdf4) into master (1d5d873) will not change coverage.
The diff coverage is 60.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #69   +/-   ##
=======================================
  Coverage   33.62%   33.62%           
=======================================
  Files          19       19           
  Lines        2091     2091           
=======================================
  Hits          703      703           
  Misses       1388     1388           
Files Coverage Δ
pspy/sph_tools.py 28.91% <60.00%> (ø)

@xgarrido
Copy link
Collaborator Author

Here is a summary just to not to forget what have been done:

  • the current pixell version 0.19.2 has fixed different bugs that makes the unit test on ubuntu passing without problem (and with no changes or no new generation of reference data)
  • on mac os, the issue is not the tests by themself but the way ducc is installed. The error is how the threads are used and I haven't found a way (compile ducc from sources for instance) or an option (see https://gitlab.mpcdf.mpg.de/mtr/ducc/-/blob/ducc0/setup.py#L22) to make it working.

@xgarrido
Copy link
Collaborator Author

xgarrido commented Oct 5, 2023

Unit tests are all passing on linux as well as on mac. When using different C++ compilers namely gcc vs. clang it raises an error due to the conversion of OMP_NUM_THREADS env. variable into long int by ducc. Basically this line in pixell set the DUCC0_NUM_THREADS variable to OMP_NUM_THREADS and, in case the OMP_NUM_THREADS is not set the utils.getenv of pixell sets the value to None. When ducc converts the string None into long int via this function https://gitlab.mpcdf.mpg.de/mtr/ducc/-/blob/ducc0/src/ducc0/infra/threading.cc#L108 it's actually impossible : with GNU compiler on linux, no error is raised and actually the value returned by mystrtol is zero but with clang, it raises an error.

Within our unit test, we set the OMP_NUM_THREADS to zero but I think pixell should set the default returned value from utils.getenv to zero something like

utils.setenv("DUCC0_NUM_THREADS", utils.getenv("OMP_NUM_THREADS", 0), keep=True)

@amaurea
Copy link

amaurea commented Oct 5, 2023

This should be fixed in pixell 0.20.3. It's fixed a bit differently, by making setenv not try to initialize DUCC0_NUM_THREADS at all if OMP_NUM_THREADS is undefined. That's how it was supposed to work in the first place, but there was a logic error in utils.setenv.

@xgarrido
Copy link
Collaborator Author

xgarrido commented Oct 5, 2023

@thibautlouis I think we are good to go and to move pspy to pixell 0.20.3 and ducc. We will update numpy when the f2py bug will be fixed (next release 1.26.1 I guess, see #78 (comment))

@xgarrido
Copy link
Collaborator Author

As expected numpy 1.26.1 fixes the issues with fortran compilation. There is no more stopper.

@thibautlouis thibautlouis merged commit 39a7259 into master Jan 9, 2024
@xgarrido xgarrido deleted the fix-testing branch January 9, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants