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

Upgrade cvode #286

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

Upgrade cvode #286

wants to merge 10 commits into from

Conversation

spco
Copy link
Collaborator

@spco spco commented Jun 14, 2018

No description provided.

@spco
Copy link
Collaborator Author

spco commented Jun 22, 2018

Should this be a breaking change, or perhaps use preprocessor directives? It might be useful to change the wiki to tell users to install cvode into a cvode2/3 directory.

@codecov
Copy link

codecov bot commented Aug 22, 2018

Codecov Report

Merging #286 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
+ Coverage   89.89%   89.91%   +0.02%     
==========================================
  Files          14       14              
  Lines        2009     2013       +4     
==========================================
+ Hits         1806     1810       +4     
  Misses        203      203
Flag Coverage Δ
#build 65.01% <37.5%> (-0.1%) ⬇️
#tests 89.36% <100%> (+0.02%) ⬆️
#unittests 30.5% <25%> (-0.02%) ⬇️
Impacted Files Coverage Δ
src/atchem2.f90 91.63% <100%> (+0.13%) ⬆️
src/outputFunctions.f90 88.15% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 264b1bd...b24f468. Read the comment docs.

@rs028
Copy link
Collaborator

rs028 commented Aug 22, 2018

Are we dealing with different versions of CVODE or we just ignore that and let the user deal with it if they need to rerun older models? Probably the latter would be the less messy solution.

@spco
Copy link
Collaborator Author

spco commented Aug 22, 2018

The latter is certainly the less messy and easiest from a development point of view. I'd suggest we go with that, but it might sway us into having to make this a 2.0 release, perhaps? It's quite a big compatibility change.

@rs028
Copy link
Collaborator

rs028 commented Aug 22, 2018

You mean this PR was not meant to be merged with #303? I am not exceedingly worried about the versioning, tbh.

I think it is okay to overwrite CVODE with the new version. We just have to document the behaviour. Users who want to use a previous version of either Atchem or CVODE are probably also skilled enough to install them by themselves.

@spco
Copy link
Collaborator Author

spco commented Aug 22, 2018

Upon reflection, I think this should probably go into v1.1 actually, so it can go before #303. I agree with your rationale for not caring about backwards compatibility at this stage.

@spco
Copy link
Collaborator Author

spco commented Aug 23, 2018

So unfortunately this seems to break the carefully-balanced behaviour tests in quite a few places. Essentially, the solver change means we get differences in 8 linux tests (and 1 of the MacOS for good measure too). I'm not sure that I want to go through the whole rigmarole of tweaking the tests to pass - that seems rather a backwards way to do things. Perhaps it's better that I do some more unit testing and then focus on getting some of the exact-solution tests out of the door?

@rs028
Copy link
Collaborator

rs028 commented Aug 23, 2018

Yes, I think that would make more sense. I don't mind postponing the upgrade to CVODE. I'd rather have solid results.

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