ginkgo icon indicating copy to clipboard operation
ginkgo copied to clipboard

Relative stopping criterion

Open keldu opened this issue 3 years ago • 2 comments

Regarding #1051 this would fix the edge case with an rhs vector filled with zeroes.

Used '<=' instead of '<' in residual operations. Test cases will be added

keldu avatar Jul 14 '22 15:07 keldu

Codecov Report

Patch coverage is 100.00% of modified lines.

:exclamation: Current head a9c1495 differs from pull request most recent head aa6ef60. Consider uploading reports for the commit aa6ef60 to get more accurate results

Files Changed Coverage
omp/stop/residual_norm_kernels.cpp ø
reference/stop/residual_norm_kernels.cpp 100.00%

:loudspeaker: Thoughts on this report? Let us know!.

codecov[bot] avatar Jul 14 '22 21:07 codecov[bot]

@keldu what's the status of this pull request? It seems to be good for review.

yhmtsai avatar Mar 09 '23 14:03 yhmtsai

Quick question, this currently stands +4000 LOC changes and 174 files changed, which seems to be to much for fixing an edge case. Could it be that it is not based on dev?

greole avatar Oct 11 '23 12:10 greole

NVM, I thought this would be ready for review.

greole avatar Oct 11 '23 12:10 greole

So. I'm not fully sure about the tests. I did a slightly small one, but I am wholly unsure if it even works. What's missing anyway for now where I am stumped. Since I keep coming back without gaining any ground.

  • [ ] Check all comments for the res_norm < \tau since we now do res_norm <= \tau
  • [ ] Check test and add proper test.

keldu avatar Oct 16 '23 20:10 keldu

And I misclicked. It is not ready for review :facepalm:

The most enlightening parts are the examples. And those basically only contain solver examples.

keldu avatar Oct 16 '23 20:10 keldu

Quick question, this currently stands +4000 LOC changes and 174 files changed, which seems to be to much for fixing an edge case. Could it be that it is not based on dev?

Yeah. I rebase'd on an incorrect develop branch. Caused a temporary issue which I didn't realise in time. I guess rebasing on origin/develop is the fix for that.

keldu avatar Oct 16 '23 21:10 keldu

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

100.0% 100.0% Coverage
11.1% 11.1% Duplication

warning The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17. Read more here

sonarqubecloud[bot] avatar Oct 23 '23 23:10 sonarqubecloud[bot]

I'm pretty sure my test is wrong btw. The problem mostly lies with the fact that I don't know what some of the functions do. And the examples don't cover initalizing already solved systems by hand.

keldu avatar Oct 26 '23 15:10 keldu

Miss the implicit residual test. I think one of abs/rel should be enough? because they all go through the same kernel and the baseline calculation should be also tested in the existing tests.

I've added a rel criterion. Still not sure about my previous comment.

keldu avatar Oct 26 '23 15:10 keldu

@keldu A simple way to check the test does what it's supposed to to is to revert the other changes and only leave the new test in, it should fail.

upsj avatar Oct 30 '23 18:10 upsj

format!

keldu avatar Nov 02 '23 14:11 keldu

LGTM. Some compiling issues and the tests need fix. Could you also rearrange the commit history by combining commits with the purpose? Also formatting in the end. I do not change my reviews status yet until the tests are correct.

I mean I could generally squash them? It's not a big change, so I would prefer to do it that way.

keldu avatar Nov 02 '23 15:11 keldu

format!

keldu avatar Nov 02 '23 15:11 keldu

format!

yhmtsai avatar Nov 03 '23 00:11 yhmtsai

the first commit adds the tests which are failed in pipeline https://gitlab.com/ginkgo-project/ginkgo-public-ci/-/pipelines/1059541223 because it requires <= not <. adding <= and updating the doc are in the following commit.

yhmtsai avatar Nov 03 '23 02:11 yhmtsai

format!

yhmtsai avatar Nov 03 '23 03:11 yhmtsai