Skip to content

Conversation

@lkeegan
Copy link
Contributor

@lkeegan lkeegan commented Mar 14, 2023

Description

  • add ASTNode::replaceArguments which replaces all supplied arguments at the same time
  • modify SBMLTransforms::replaceBvars to use this instead of replacing arguments sequentially
  • refactor shared functionality into copyNode
  • modify test case from using mul to minus to show some results are still incorrect after Add test case for replaceFD issue #299 #300

Motivation and Context

SBMLTransforms::replaceFD gave incorrect results in some cases.
This was due to SBMLTransforms::replaceBvars replacing each bvar sequentially (#299)
This PR instead substitutes all bvars simultaneously to avoid these incorrect results.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Change in documentation

Checklist:

  • I have updated all documentation necessary.
  • I have checked spelling in (new) comments.

Testing

  • Testing is done automatically and codecov shows test coverage
  • This cannot be tested automatically

lkeegan added 2 commits March 14, 2023 14:02
- add `ASTNode::replaceArguments` to replace all arguments at the same time
- refactor shared functionality into `copyNode`
- modify `SBMLTransforms::replaceBvars` to use this instead of replacing arguments sequentially
Copy link
Member

@fbergmann fbergmann left a comment

Choose a reason for hiding this comment

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

that does look better, thanks

@fbergmann fbergmann merged commit a664e47 into sbmlteam:development Mar 14, 2023
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