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

Improve Coverage #99

Merged
merged 15 commits into from
Feb 9, 2016
Merged

Improve Coverage #99

merged 15 commits into from
Feb 9, 2016

Conversation

mbillingr
Copy link
Member

Addresses #96

@cbrnr
Copy link
Contributor

cbrnr commented Feb 2, 2016

LGTM. I wonder why the coverage increase isn't shown here.

@mbillingr
Copy link
Member Author

It is shown when you click on "Show all checks".

@cbrnr
Copy link
Contributor

cbrnr commented Feb 8, 2016

Any idea why the bisection test fails?

Also, since we'll be treating the issues with parallelization in #30, maybe you should also remove the corresponding tests for now? Or is this not related?

@mbillingr
Copy link
Member Author

This is not related. The problem is here:

    if trform(b) >= maxdelta and verbose:
        print('Bisection: could not find initial interval.')
        print(' ********* Delta set to zero! ************ ')
        return 0

The test checks if the function returns 0 because b is too high. This works locally because I have set verbose. On travis verbose is False so it does not return 0.

@cbrnr
Copy link
Contributor

cbrnr commented Feb 8, 2016

LGTM. We're almost at 90%. Can you add a test to cover the remaining 0.035%? This would be great from a psychological perspective :-).

@mbillingr
Copy link
Member Author

I've meant this as a sort of open ended PR, anyway :)

@cbrnr
Copy link
Contributor

cbrnr commented Feb 9, 2016

We need to merge it at some point - otherwise the increased coverage remains in this branch.That's why I think 90% would be a good threshold to merge this (and continue increasing coverage in another PR later).

@mbillingr
Copy link
Member Author

Right.

@mbillingr mbillingr added this to the 0.2 Release milestone Feb 9, 2016
@cbrnr cbrnr mentioned this pull request Feb 9, 2016
@mbillingr
Copy link
Member Author

When writing these tests I came across a bug in CSPVARICA (we forgot to fix data orientation in one place).

Hooray for testing!

@cbrnr
Copy link
Contributor

cbrnr commented Feb 9, 2016

Nice! I think we can merge this now, right?

mbillingr added a commit that referenced this pull request Feb 9, 2016
@mbillingr mbillingr merged commit 5819d89 into scot-dev:master Feb 9, 2016
@mbillingr mbillingr deleted the coverage branch February 9, 2016 12:42
@mbillingr mbillingr restored the coverage branch February 9, 2016 12:42
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.

None yet

2 participants