Skip to content

Conversation

@benegee
Copy link
Collaborator

@benegee benegee commented Jul 5, 2023

Not intended for merging, rather for getting used to Trixi.

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #45 (1af0098) into main (68adae0) will increase coverage by 1.80%.
Report is 4 commits behind head on main.
The diff coverage is 76.54%.

@@            Coverage Diff             @@
##             main      #45      +/-   ##
==========================================
+ Coverage   69.89%   71.69%   +1.80%     
==========================================
  Files           6        8       +2     
  Lines         186      265      +79     
==========================================
+ Hits          130      190      +60     
- Misses         56       75      +19     
Flag Coverage Δ
unittests 71.69% <76.54%> (+1.80%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/trixi.f90 81.81% <ø> (ø)
examples/trixi_controller_data.f90 73.68% <73.68%> (ø)
src/trixi.c 67.34% <76.92%> (+0.68%) ⬆️
examples/trixi_controller_data.c 80.00% <80.00%> (ø)

@benegee benegee requested a review from sloede July 14, 2023 12:45
@sloede sloede marked this pull request as ready for review July 14, 2023 13:32
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

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

Overall looks quite nice to me! I've done a "somewhat" in-depth review, so there a lots of small details I noticed. But I'll leave it up to you if you want to merge this fast now and clean up later, or do it thoroughly right now.

@sloede
Copy link
Member

sloede commented Jul 14, 2023

In general I noticed that you do not seem to test the new controllers... are they even built in the tests?

@benegee benegee marked this pull request as draft August 14, 2023 08:04
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
@benegee benegee requested a review from sloede August 14, 2023 09:07
@benegee benegee marked this pull request as ready for review August 14, 2023 09:07
sloede
sloede previously approved these changes Aug 14, 2023
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

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

LGTM!

@sloede
Copy link
Member

sloede commented Aug 14, 2023

Coverage is down because the C API functions are not used. I think you should call the new test program in our CI testing to fix this, e.g., by adding corresponding calls here and here.

@sloede sloede enabled auto-merge (squash) August 14, 2023 09:50
@sloede sloede merged commit f167c86 into main Aug 14, 2023
@sloede sloede deleted the bg/get_cell_averages branch August 14, 2023 10:08
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.

3 participants