You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Refactored parameter handling in prepareDatahandleForIntegration (renamed 'solver' to 'integrator'!), updated optionlists to the most recent version (all functions were renamed) and replaced relative paths in initPaths and clearPaths with absolute paths.
Changelog
Cleaned up parameter handling in prepareDatahandleForIntegration by introducing a new function.
Renamed the 'solver' parameter to 'integrator'
The 'integrator' parameter now also accepts char arrays as well as function handles
Changed default integrator to ode45 and adjusted default tolerance options
Added warning message if a user does not specify a parameter and the default is used instead
Updated optionlists to the most recent version in the mmtools repository
All functions now have an 'ol' prefix in their name to avoid shadowing certain MATLAB functions
Some additional improvements and new functionality
Replaced relative paths in initPaths and clearPaths with absolute paths
This prevents issues where someone may call initPaths from a different working directory (i.e. not the IFDIFF project root directory) which would lead to unintended behavior
Also adjusted test cases which need to manipulate the MATLAB search path
Notes
The speedtracker will report that the results of the benchmarks have changed compared to the public branch. This is caused by the fact that the benchmarks use the 'integrator' parameter which matches the implementation in this branch, but differs from the one in the public branch where 'solver' is used instead. Therefore, the benchmarks will use the default integrator in the public branch (ode15s) and the integrator that was actually passed as a parameter (ode45) in this branch. Manually changing the benchmarks to use ode15s instead will give the same result for both commits.
Nice code! A few questions and thoughts that arose while reading it:
There have been added history comments that inform the reader about how some code has changed
(cf. Tools/optionlists/olAssertOptionlist.m, l.15 for example).
Do we want these kind of comments?
The code is not backward-compatible as it is now. A user who has written code for an older version will have to replace 'solver' by 'integrator' for all their files. Also, the speedtracker has problems with this change as you mentioned under 'Notes'.
We should talk about how much backward-compatibilty we want to ensure and how we enable the speedtracker to execute versions with different interfaces if necessary.
prepareDatahandleForIntegration_setUpIntegrator.m, line 21:
It would be nice to read default settings from the config (makeConfig.m).
Nice code! A few questions and thoughts that arose while reading it:
There have been added history comments that inform the reader about how some code has changed
(cf. Tools/optionlists/olAssertOptionlist.m, l.15 for example).
Do we want these kind of comments?
The code is not backward-compatible as it is now. A user who has written code for an older version will have to replace 'solver' by 'integrator' for all their files. Also, the speedtracker has problems with this change as you mentioned under 'Notes'.
We should talk about how much backward-compatibilty we want to ensure and how we enable the speedtracker to execute versions with different interfaces if necessary.
prepareDatahandleForIntegration_setUpIntegrator.m, line 21:
It would be nice to read default settings from the config (makeConfig.m).
Thanks for the review and suggestions! Here are my thoughts:
In general, we probably don't need these kind of comments for IFDIFF code, since we already have the history in git. However, the optionlists functionality comes from the external mmtools repo, so it might be helpful to keep such comments in this case. Either way, I wouldn't bother changing anything in the optionslists code because it is only used as an external helper tool in IFDIFF. If we do actually need to modify something in the future (e.g. to fix a bug or add new functionality), then we should address that in the mmtools repo first.
I believe we already discussed and approved this interface change, but I also don't mind bringing it up again in tomorrow's meeting just in case. It is a breaking interface change after all, so I agree with you that we should be cautious.
About the speedtracker: I think it's a good thing that the speedtracker reports that something has changed (something major did in fact change). I'm not sure if it would be worth it to add some kind of functionality that would allow you to run benchmarks with versions that have different interfaces. It seems to me like it would require a significant amount of work with potentially not a lot of benefit. Still, I think it's definitely also worth discussing this tomorrow, so thanks for bringing that up.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactored parameter handling in
prepareDatahandleForIntegration(renamed 'solver' to 'integrator'!), updated optionlists to the most recent version (all functions were renamed) and replaced relative paths ininitPathsandclearPathswith absolute paths.Changelog
prepareDatahandleForIntegrationby introducing a new function.initPathsandclearPathswith absolute pathsinitPathsfrom a different working directory (i.e. not the IFDIFF project root directory) which would lead to unintended behaviorNotes
The speedtracker will report that the results of the benchmarks have changed compared to the public branch. This is caused by the fact that the benchmarks use the 'integrator' parameter which matches the implementation in this branch, but differs from the one in the public branch where 'solver' is used instead. Therefore, the benchmarks will use the default integrator in the public branch (ode15s) and the integrator that was actually passed as a parameter (ode45) in this branch. Manually changing the benchmarks to use ode15s instead will give the same result for both commits.