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

Further improve coverage #348

Merged
merged 8 commits into from
Nov 18, 2020
Merged

Further improve coverage #348

merged 8 commits into from
Nov 18, 2020

Conversation

sloede
Copy link
Member

@sloede sloede commented Nov 18, 2020

I modified the positivity tests such that the limiters in 1D and 2D are actually triggered.

@sloede sloede requested a review from ranocha November 18, 2020 14:09
@ranocha
Copy link
Member

ranocha commented Nov 18, 2020

Did you have a look at the numerical solutions or CFL magic?

@sloede
Copy link
Member Author

sloede commented Nov 18, 2020

Did you have a look at the numerical solutions or CFL magic?

Sorry, no, not yet. Do you think you can do it, or should we just merge now and check it later?

@ranocha
Copy link
Member

ranocha commented Nov 18, 2020

We can merge now and check later 👍

@sloede
Copy link
Member Author

sloede commented Nov 18, 2020

If we're to achieve 97% coverage again, we need to cover 30 more lines. I am looking at it right now...

@sloede sloede requested a review from ranocha November 18, 2020 19:36
@sloede
Copy link
Member Author

sloede commented Nov 18, 2020

If this doesn't push coverage beyond 97%, I'll give up on this goal before v0.3... Note that some checks now already make use of the Taal way: Instead of doing only integral tests, I am adding a bunch of unit tests for many methods such as entropy or pressure. I hope that's ok - if not, I propose to proceed anyway and open an issue for it (or, of course, any of you should feel free to add integral checks instead).

Copy link
Member

@ranocha ranocha left a comment

Choose a reason for hiding this comment

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

Thanks!

Do you think it would be helpful to add some comments like the following one to test_manual.jl?

Constructing indicators/controllers using the parameters below doesn't make sense. It's just useful to run basic tests of show methods.

@sloede
Copy link
Member Author

sloede commented Nov 18, 2020

Do you think it would be helpful to add some comments like the following one to test_manual.jl?

Constructing indicators/controllers using the parameters below doesn't make sense. It's just useful to run basic tests of show methods.

Yes, good idea 👍. I'll do so once the tests here have passed and before the merge.

@sloede sloede merged commit 2d50cdf into master Nov 18, 2020
@sloede sloede deleted the further-improve-coverage branch November 18, 2020 21:49
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