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

Fix narrowing errors by replacing int by ssize_t #725

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maresb
Copy link
Contributor

@maresb maresb commented Apr 21, 2024

Ref: pymc-devs/sunode#46 (comment)

Description

I don't actually know any C++ or have any idea if this is a sensible change. But it seems like it may fix some obscure errors on Mac with sunode.

Related Issue

  • Closes #
  • Related to #

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

Copy link

codecov bot commented Apr 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.76%. Comparing base (58aad80) to head (7785dc8).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #725   +/-   ##
=======================================
  Coverage   80.76%   80.76%           
=======================================
  Files         162      162           
  Lines       46715    46715           
  Branches    11427    11427           
=======================================
  Hits        37729    37729           
  Misses       6736     6736           
  Partials     2250     2250           
Files Coverage Δ
pytensor/tensor/elemwise_cgen.py 95.34% <ø> (ø)

@ricardoV94
Copy link
Member

ricardoV94 commented Apr 21, 2024

Not sure about this, gonna need to read about it. It seems that stuff were purposefully using int?

@maresb
Copy link
Contributor Author

maresb commented Apr 21, 2024

Perhaps, I don't really understand much myself.

@aseyboldt
Copy link
Member

The original int is I think really weird, to a point of just being wrong. Pointer offsets shouldn't be a int, I guess that leads to compiler errors in newer versions? On most machines they are 32 bit, but all strides etc should usually be the same size as pointers, which are 64 bit.
I guess they were an int originally because size_t can only be positive.
ptrdiff_t should I think be the right type however, not ssize_t, even though in practice they are probably implemented as the same thing most of the time. ssize_t is not guaranteed to be able to represent anything smaller than -1, and it is POSIX only as well.

@maresb
Copy link
Contributor Author

maresb commented Apr 23, 2024

Great, thanks a lot @aseyboldt for the "pointer" to ptrdiff_t. 🙂

I'm busy with some other stuff at the moment, but I'll come back to this. I'd like to enable some basic tests on macos (which I expect to fail), and then we'll be able to test properly.

@maresb
Copy link
Contributor Author

maresb commented Jun 17, 2024

Was hoping for an easy way out with conda-forge/sunode-feedstock#35, but it didn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants