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 integer overflow #354

Closed
wants to merge 10 commits into from
Closed

Fix integer overflow #354

wants to merge 10 commits into from

Conversation

wilfonba
Copy link
Contributor

@wilfonba wilfonba commented Feb 23, 2024

Description

Fixes integer overflow for simulations with more than 2^31 cells.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Something else

Scope

  • This PR comprises a set of related changes with a common goal

If you cannot check the above box, please split your PR into multiple PRs that each have a common goal.

@sbryngelson
Copy link
Member

Something about some of these changes doesn't quite seem right to me (I could be wrong). For example, if you write int(p), this shouldn't do anything if p is already int. Likewise for other quantities.

@wilfonba
Copy link
Contributor Author

int(p) casts an 8-byte integer p to a 4-byte integer. The MPI routines only accept 4-byte integers.

@sbryngelson
Copy link
Member

int(p) casts an 8-byte integer p to a 4-byte integer. The MPI routines only accept 4-byte integers.

Got it - though I see a couple other ones, like for s_compute_finite_difference_coefficients.

@wilfonba
Copy link
Contributor Author

wilfonba commented Feb 23, 2024

I didn't change the integer types in simulation because the global cell count is never computed in simulation. s_compute_finite_difference_coefficients is used in both simulation and post_process and expects a 4-byte integer.

@sbryngelson
Copy link
Member

sbryngelson commented Feb 23, 2024

I didn't change the integer types in simulation because the global cell count is never computed in simulation. s_compute_finite_difference_coefficients is used in both simulation and post_process and expects a 4-byte integer.

Does it not automatically up (or down) cast the input precision? That would make this much easier to maintain, otherwise every time one of the "special" variables (e.g., p) is used in both simulation and another code (pre/post process), the developer needs to know that they need to do int(p).

@wilfonba
Copy link
Contributor Author

It does not automatically cast the inputs. I'll just make simulation use 8-byte integers for m, n, and p, then everything will be the same. The only reason I didn't was because adding all the casting in the MPI routines would be tedious.

@sbryngelson
Copy link
Member

It does not automatically cast the inputs. I'll just make simulation use 8-byte integers for m, n, and p, then everything will be the same. The only reason I didn't was because adding all the casting in the MPI routines would be tedious.

How is this different than what you already did?

@wilfonba
Copy link
Contributor Author

I'll just explain it at our meeting this afternoon. It'll be easier that way.

@wilfonba wilfonba closed this Feb 24, 2024
@wilfonba wilfonba deleted the bugFixes branch February 24, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants