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

Unify DGMultiMesh constructor signature, update overview.md #1386

Merged
merged 12 commits into from
Apr 10, 2023

Conversation

jlchan
Copy link
Contributor

@jlchan jlchan commented Apr 6, 2023

Addresses part of #1194.

This would be a breaking change (introduces a new deprecation).

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! Did you check that the deprecation works as expected?

@jlchan
Copy link
Contributor Author

jlchan commented Apr 6, 2023

Not yet - let me convert this to draft, I don't think it's quite ready.

@jlchan jlchan marked this pull request as draft April 6, 2023 14:34
@jlchan jlchan marked this pull request as ready for review April 7, 2023 04:06
@jlchan
Copy link
Contributor Author

jlchan commented Apr 7, 2023

The deprecation does seem to work. Tests were failing for some other reasons, those should be fixed now.

ranocha
ranocha previously approved these changes Apr 7, 2023
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

@jlchan
Copy link
Contributor Author

jlchan commented Apr 7, 2023

Seems like some tests are failing due to the elixirs triggering a deprecation warning and trixi_include? Here's a stacktrace from one of the last failures:

Content of `stderr`:
[ Info: You just called `trixi_include`. Julia may now compile the code, please be patient.
┌ Warning: `DGMultiMesh(dg::DGMulti{NDIMS}; cells_per_dimension, kwargs...) where NDIMS` is deprecated, use `DGMultiMesh(dg, cells_per_dimension; kwargs...)` instead.
│   caller = ip:0x0
└ @ Core :-1

elixir_euler_weakform_periodic.jl: Test Failed at /home/runner/work/Trixi.jl/Trixi.jl/test/test_trixi.jl:167
  Expression: occursin(r"^(WARNING: replacing module .+\.\n)*$", stderr_content)
   Evaluated: occursin(r"^(WARNING: replacing module .+\.\n)*$", "┌ Warning: `DGMultiMesh(dg::DGMulti{NDIMS}; cells_per_dimension, kwargs...) where NDIMS` is deprecated, use `DGMultiMesh(dg, cells_per_dimension; kwargs...)` instead.\n│   caller = ip:0x0\n└ @ Core :-1\n")
Stacktrace:
 [1] macro expansion
   @ /opt/hostedtoolcache/julia/1.8.5/x64/share/julia/stdlib/v1.8/Test/src/Test.jl:464 [inlined]
 [2] macro expansion
   @ ~/work/Trixi.jl/Trixi.jl/test/test_trixi.jl:167 [inlined]
 [3] macro expansion
   @ ~/work/Trixi.jl/Trixi.jl/test/test_dgmulti_2d.jl:124 [inlined]
 [4] macro expansion
   @ /opt/hostedtoolcache/julia/1.8.5/x64/share/julia/stdlib/v1.8/Test/src/Test.jl:1363 [inlined]
 [5] top-level scope
   @ ~/work/Trixi.jl/Trixi.jl/test/test_dgmulti_2d.jl:124

I replaced all the example and Literate elixirs with the new constructor. Hopefully it helps

@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Merging #1386 (3384557) into main (8a182e2) will increase coverage by 6.70%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1386      +/-   ##
==========================================
+ Coverage   86.99%   93.69%   +6.70%     
==========================================
  Files         344      344              
  Lines       28526    28560      +34     
==========================================
+ Hits        24816    26758    +1942     
+ Misses       3710     1802    -1908     
Flag Coverage Δ
unittests 93.69% <100.00%> (+6.70%) ⬆️

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

Impacted Files Coverage Δ
examples/dgmulti_2d/elixir_advection_diffusion.jl 100.00% <ø> (ø)
...multi_2d/elixir_advection_diffusion_nonperiodic.jl 100.00% <ø> (ø)
.../dgmulti_2d/elixir_advection_diffusion_periodic.jl 100.00% <ø> (ø)
...les/dgmulti_2d/elixir_euler_brown_minion_vortex.jl 100.00% <ø> (+100.00%) ⬆️
...ti_2d/elixir_euler_kelvin_helmholtz_instability.jl 100.00% <ø> (+100.00%) ⬆️
...lti_2d/elixir_euler_rayleigh_taylor_instability.jl 100.00% <ø> (+100.00%) ⬆️
examples/dgmulti_2d/elixir_euler_weakform.jl 100.00% <ø> (ø)
...ples/dgmulti_2d/elixir_navierstokes_convergence.jl 100.00% <ø> (ø)
...gmulti_2d/elixir_navierstokes_lid_driven_cavity.jl 100.00% <ø> (ø)
...les/dgmulti_3d/elixir_euler_taylor_green_vortex.jl 100.00% <ø> (+100.00%) ⬆️
... and 4 more

... and 122 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jlchan
Copy link
Contributor Author

jlchan commented Apr 7, 2023

@ranocha I added a @deprecate statement for the old constructor - would you still consider this a breaking change?

@ranocha
Copy link
Member

ranocha commented Apr 8, 2023

It is not breaking if the constructor is just deprecated but still works

@ranocha
Copy link
Member

ranocha commented Apr 8, 2023

But we should mention it in NEWS.md

@jlchan jlchan requested a review from ranocha April 8, 2023 05:08
@jlchan
Copy link
Contributor Author

jlchan commented Apr 8, 2023

Agreed; added a note in NEWS.md about this.

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!

@ranocha ranocha merged commit 12fe7fb into trixi-framework:main Apr 10, 2023
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