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

New features #155

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

New features #155

wants to merge 2 commits into from

Conversation

sixpearls
Copy link

I made a few changes to the CVODE interface that I needed for a project that I wanted share with the main distribution in case it was of interest. Since I was merely prototyping I have several features/changes that committed on top of each other:

  • make options dict and and with_userdata flag public (done for prototyping, not attached to this one)
  • the ability to pass userdata to the jacfn (I did not actually end up using this for my project, but it seemed like someone else had it almost working)
  • allow max_step_size to be changed after the solver has been instantiated (used in my project)
  • allow arbitrary values of tstop option to be used rather than enforcing strictly positive (used in my project)
  • adding a wrapper to the CVodeGetRootInfo method which "returns an array showing which functions were found to have a root" according to the SUNDIALS docs (used in my project)

Are any/all of these changes of interest? If so, would a monolithic PR be acceptable or do they need to be split into individual PRs? Once the features are approved, I can update the docs and add tests as appropriate.

Best,
Ben

@sixpearls
Copy link
Author

Hi @aragilar, it seems like you are the current maintainer of this package. When you have the chance could you provide input on this PR? Thanks,

@aragilar
Copy link
Collaborator

I think if you split them up it would make it easier to review. At a quick glance:

  • CVodeGetRootInfo seems like something useful to add
  • I'm not sure if the changes to tstop and max_step_size are safe, it would be good to confirm that both changes don't break some underlying assumption (otherwise I have no objection).
  • I'm not sure why jacfn didn't include the userdata passing, there may have been a reason for that (though maybe it was that there was an alternative way of passing in data)?
  • I'm wary of making both options and with_userdata public, as they are internal state which should be set via the various functions and helpers.

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.

2 participants