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.

Sign up for GitHub

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

Jump to bottom

Ode solver with sundials third party library. #1109

Merged
merged 53 commits into from
May 4, 2016
Merged

Ode solver with sundials third party library. #1109

merged 53 commits into from
May 4, 2016

Conversation

endJunction
Copy link
Member

@endJunction endJunction commented Mar 18, 2016 •
edited
Loading

This is an adaptation and cleanup of Christophs ode solver interface.

Todo (unordered list):

  • (non-monadic) check error
  • Fix memleaks
  • Test suitability for TES
  • Documentation
  • Changlog
  • Check CMake for sundials.
  • Rework getYDot

@endJunction endJunction added this to the TES Process milestone Mar 18, 2016
chleh
chleh reviewed Mar 18, 2016
View reviewed changes
Tests/MathLib/TestCVode.cpp
@@ -121,7 +121,7 @@ TEST(MathLibCVodeTest, ZeoliteAdsorption)

ode_solver->setFunction(f_zeolite, nullptr);

ode_solver->setIC(0.0, {pV, C});
ode_solver->setIC(0.0, {{pV, C}});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only for my understanding: That's due to some c++11 std::array constructor implementation "bug", right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can be. I do remember something about std::array. On the other hand std::array is just struct, so it could be related to struct's initialization.

@endJunction endJunction force-pushed the ode-solver branch 2 times, most recently from 235dfc9 to 77c2de3 Compare March 18, 2016 14:36
chleh
chleh reviewed Mar 19, 2016
View reviewed changes
MathLib/ODE/CVodeSolver.cpp
return successful ? 0 : 1;
};

// TODO: check not run twice! move this call somewhere else
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now the comment is not needed anymore. ✅

@chleh
Copy link
Collaborator

chleh commented Mar 19, 2016

@endJunction, I checked your branch yesterday evening at commit 77c2de3.
You didn't do any major API changes, so this branch—as expected—still suits my needs.

I only have three simple suggestions:

  • Please add overloads to setIC() and getYDot() which take std::array<double, N> const&, because then one can use those functions like ode_solver.setIC(1.0, { 2.0, 3*x, cos(y) }). ✅
  • I don't know if it is somewhere documented in our code that sundials expects column-major matrix storage order. At least it should be explicitly documented. Furthermore, it would probably be good if you included the storage order parameter Eigen::ColMajor explicitly into the MappedMatrix and MappedConstMatrix typedefs. ✅
  • You might want to move the classes Handles and ConcreteOdeSolver out of OdeSolverFactory.h to two separate files. That would make understanding the code and file structure slightly easier. ✅

@endJunction endJunction force-pushed the ode-solver branch 2 times, most recently from a5c6080 to 392bcd3 Compare March 21, 2016 16:58
@ogsbot
Copy link
Member

ogsbot commented Mar 21, 2016

Jenkins: OGS-6/Linux-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Linux-PRs/1484/

@ogsbot
Copy link
Member

ogsbot commented Mar 21, 2016

Jenkins: OGS-6/Gui/Mac-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Gui/job/Mac-PRs/1349/

@ogsbot
Copy link
Member

ogsbot commented Mar 21, 2016

Jenkins: OGS-6/Mac-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Mac-PRs/1368/

@ogsbot
Copy link
Member

ogsbot commented Mar 21, 2016

Jenkins: OGS-6/Gui/Gui-Linux-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Gui/job/Gui-Linux-PRs/1458/

@ogsbot
Copy link
Member

ogsbot commented Mar 21, 2016

Jenkins: OGS-6/Win-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Win-PRs/1370/

@ogsbot
Copy link
Member

ogsbot commented Mar 21, 2016

Jenkins: OGS-6/Gui/Win-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Gui/job/Win-PRs/1348/

@bilke
Copy link
Member

bilke commented Apr 5, 2016

@endJunction Feel free to git cherry-pick bilke/ogs@2d6221bc7a5

endJunction
endJunction reviewed Apr 6, 2016
View reviewed changes
MathLib/CMakeLists.txt
if (CVODE_FOUND)
target_link_libraries(MathLib INTERFACE
GeoLib
logog
Copy link
Member Author

@endJunction endJunction Apr 6, 2016 •
edited by chleh
Loading

Choose a reason for hiding this comment

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

Still needs GeoLib and logog ✅

Copy link
Collaborator

Choose a reason for hiding this comment

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

done.

chleh and others added 25 commits May 4, 2016 09:34
@chleh @endJunction
[MaL] added comment
da755d9
@chleh @endJunction
[MaL] unified template param order
cc66676
@chleh @endJunction
[MaL] use std::function
b4c4a06
@chleh @endJunction
[T] adapted to std::function
50d61d9
@chleh @endJunction
[MaL] renamed and moved FunctionHandles
02dd458
@chleh @endJunction
[MaL] solve returns success code
034e434
@chleh @endJunction
[T] added assertion, use logog
0f83123
@chleh @endJunction
[MaL] reformatted stats
b77f6e7
@chleh @endJunction
[MaL] added comment, added assert, init values
8cd4881
@chleh @endJunction
[T] check results are within tolerances
ac3bab8
@chleh @endJunction
[MaL] pass Eigen::Map's by reference
75ae4de
@chleh @endJunction
[MaL] changed spelling ode -> ODE
8cf0f3a
@chleh @endJunction
[T] changed filename and spelling.
5d5a277
@chleh @endJunction
[MaL] moved create function to separate file
d989aa1
@chleh @endJunction
[MaL] fixed includes
58d9202
@chleh @endJunction
[T] fixed includes
acf5f8a
@chleh @endJunction
[MaL] minor reformatting, removed duplicate virtual
2d8f142
@chleh @endJunction
[MaL] removed unneeded typedefs
910bcf6
@chleh @endJunction
[MaL] added API docu
4469719
@chleh @endJunction
[MaL] removed unneeded checks
dbcdb9f
@chleh @endJunction
[MaL] Added conceptual ode-solver documentation.
8ef8964 
Added doc link on main page.
@chleh @endJunction
[MaL] abort if no ODE solver can be created.
4a088c4 
And also in the tests prevent abort if no ODE solver can be created.
@chleh @endJunction
[T] added error message
1f2e2ca
@chleh @endJunction
[MaL] added ODE namespace
358690f
@endJunction
Update Changelog.
2f6e1c4
@endJunction endJunction merged commit ba66452 into ufz:master May 4, 2016
@endJunction endJunction deleted the ode-solver branch May 4, 2016 08:33
@ogsbot
Copy link
Member

ogsbot commented Jun 19, 2020

OpenGeoSys development has been moved to GitLab.

See this pull request on GitLab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
TES Process
Development

Successfully merging this pull request may close these issues.
7 participants
@endJunction @chleh @ogsbot @bilke @wenqing @norihiro-w @TomFischer