Skip to content

Conversation

@shakedregev
Copy link
Collaborator

@shakedregev shakedregev commented Oct 7, 2025

Description

Memory allocation can be incidental in ReSolve rather than explicitly defined by the user. Multiple calls to functions that should only be called once, or calls to function without first calling a prerequisite function are allowed. Addresses (no commitment that it fully closes) #295 and #19. I'd like your opinion on whether you think it closes these issues. Currently the code allows the user to set a vector to a constant without allocating it. I am ambivalent, but I do think it should be allowed over all. Let me know your thoughts.

Proposed changes

Fixed typos, removed the ability to sloppy synch or sloppily call functions out of order.

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask. We're here
to help! This is simply a reminder of what we are going to look for before
merging your code.

  • All tests pass. Code tested on
    • CPU backend
    • CUDA backend
    • HIP backend
  • Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
  • The new code follows Re::Solve style guidelines.
  • There are unit tests for the new code.
  • The new code is documented.
  • No merge conflicts.
  • The changes are included within the changelog.

@shakedregev shakedregev self-assigned this Oct 7, 2025
@shakedregev shakedregev added the help wanted Extra attention is needed label Oct 7, 2025
@shakedregev shakedregev marked this pull request as draft October 7, 2025 14:31
@shakedregev shakedregev marked this pull request as ready for review October 7, 2025 14:50
@shakedregev shakedregev removed the help wanted Extra attention is needed label Oct 7, 2025
@pelesh pelesh added this to the Release 0.99.3 milestone Oct 9, 2025
@pelesh
Copy link
Collaborator

pelesh commented Oct 9, 2025

Let's prioritize the 0.99.2 release. This PR requires a little more thought. Also, it would be good to refactor ExampleHelper, first, because some of the methods in there are not used/needed anymore.

Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

Some suggestions on error messaging. In principle, I would tell users what (generic) procedure has faild/was called in error rather than specifying a cryptic variable name. We could use __line__ and __func__ to provide a location of the error in the code.

Comment on lines +145 to 146
assert(res_ != nullptr && "resetSystem should be called after setSystem");
A_ = A;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not touch this file for now. It needs more refactoring than this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The change is very simple and is better than what we have currently. We can debate the error message, but the user should not be allowed to reset the system without first setting it.

@shakedregev
Copy link
Collaborator Author

Thanks for the suggestions. I will implement them locally because they touch more than the comments (make sure there are no typos in variable names for example) and push them. The one exception is the suggestion I disagreed with explicitly.

@shakedregev shakedregev requested a review from pelesh November 11, 2025 17:45
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.

3 participants