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

Logging pressure solver #135

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

marinlauber
Copy link
Member

@marinlauber marinlauber commented Jun 26, 2024

This is a draft pull request that implements some changes in the pressure solver.

A sample of the pressure solver log can be found here.
WaterLily.log

Changes:

  • add @debug via LoggingExtras.jl to monitor the pressure residuals, iterations, etc.
  • remove the adaptive multilevel pressure solver and increase the size of the smallest domain to N>=8
  • add a test of the impulsive flow around a cylinder to check the pressure forces and oscillations in the pressure field.

Things to do:

  • custom logging to free the @debug macro and use @logmsg instead for the pressure logging, see https://github.com/JuliaLogging/LoggingExtras.jl
  • [ ] Type dispatch of the adaptive/non-adaptive multilevel pressure solver depending on the precision?
  • [ ] some proper pressure solve tests

src/util.jl Outdated Show resolved Hide resolved
@marinlauber marinlauber changed the title Pressure solver for Float32 Logging pressure solver and improvement for Float32 Jun 26, 2024
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is the logger used in this example?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just by calling WaterLily.logger(...) triggers the @debug macros to print the expression behind to the log file. If this is not done @debug is not evaluated. Line 37 of test_psolver.jl

Copy link
Collaborator

@weymouth weymouth Jun 28, 2024

Choose a reason for hiding this comment

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

Can you include an example of reading the file? And add a sample plot as a comment to this PR?

src/MultiLevelPoisson.jl Outdated Show resolved Hide resolved
@marinlauber marinlauber marked this pull request as ready for review June 27, 2024 07:28
@marinlauber marinlauber changed the title Logging pressure solver and improvement for Float32 Logging pressure solver Jun 28, 2024
@marinlauber
Copy link
Member Author

I am not sure what to do with the plotting script for the logger, it's really ugly and I don't want to include in into util.jl. I put it here for now...

using Plots

function plot_logger(fname="WaterLily.log")
    predictor = []; corrector = []
    open(fname,"r") do f
        readline(f) # read first line and dump it
        which = "p"
        while ! eof(f)  
            s = split(readline(f) , ",")          
            which = s[1] != "" ? s[1] : which
            push!(which == "p" ? predictor : corrector,parse.(Float64,s[2:end]))
        end
    end
    predictor = reduce(hcat,predictor)
    corrector = reduce(hcat,corrector)

    # get index of all time steps
    idx = findall(==(0.0),@views(predictor[1,:]))
    # plot inital L∞ and L₂ norms of residuals for the predictor step
    p1=plot(1:length(idx),predictor[2,idx],color=:1,ls=:dash,label="predictor initial r∞",yaxis=:log,size=(800,400),dpi=600,
            xlabel="Time step",ylabel="L∞-norm",title="Residuals",ylims=(1e-6,1e0),xlims=(0,length(idx)))
    p2=plot(1:length(idx),predictor[2,idx],color=:1,ls=:dash,label="predictor initial r₂",yaxis=:log,size=(800,400),dpi=600,
            xlabel="Time step",ylabel="L₂-norm",title="Residuals",ylims=(1e-6,1e0),xlims=(0,length(idx)))
    # plot final L∞ and L₂norms of residuals for the predictor
    plot!(p1,1:length(idx),vcat(predictor[2,idx[2:end].-1],predictor[2,end]),color=:1,lw=2,label="predictor r∞")
    plot!(p2,1:length(idx),vcat(predictor[3,idx[2:end].-1],predictor[3,end]),color=:1,lw=2,label="predictor r₂")
    # plot the MG iterations for the predictor
    p3=plot(1:length(idx),vcat(predictor[1,idx[2:end].-1],predictor[1,end]),label="predictor",size=(800,400),dpi=600,
            xlabel="Time step",ylabel="Iterations",title="MG Iterations",ylims=(0,32),xlims=(0,length(idx)))
    # get index of all time steps
    idx = findall(==(0.0),@views(corrector[1,:]))
    # plot inital L∞ and L₂ norms of residuals for the corrector step
    plot!(p1,1:length(idx),corrector[2,idx],color=:2,ls=:dash,label="corrector initial r∞",yaxis=:log)
    plot!(p2,1:length(idx),corrector[3,idx],color=:2,ls=:dash,label="corrector initial r₂",yaxis=:log)
    # plot final L∞ and L₂ norms of residuals for the corrector step
    plot!(p1,1:length(idx),vcat(corrector[2,idx[2:end].-1],corrector[2,end]),color=:2,lw=2,label="corrector r∞")
    plot!(p2,1:length(idx),vcat(corrector[3,idx[2:end].-1],corrector[3,end]),color=:2,lw=2,label="corrector r₂")
    # plot MG iterations of the corrector
    plot!(p3,1:length(idx),vcat(corrector[1,idx[2:end].-1],corrector[1,end]),label="corrector")
    # plot all together
    plot(p1,p2,p3,layout=@layout [a b c])
end


plot_logger("WaterLily.log")
savefig("WaterLily_psolver.png")

the result is this

WaterLily_psolver

Copy link

codecov bot commented Jun 28, 2024

Codecov Report

Attention: Patch coverage is 53.84615% with 6 lines in your changes missing coverage. Please review.

Project coverage is 93.28%. Comparing base (36dc237) to head (85c78b8).

Current head 85c78b8 differs from pull request most recent head bf3f2f0

Please upload reports for the commit bf3f2f0 to get more accurate results.

Files Patch % Lines
src/util.jl 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
- Coverage   94.29%   93.28%   -1.02%     
==========================================
  Files          12       12              
  Lines         526      536      +10     
==========================================
+ Hits          496      500       +4     
- Misses         30       36       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@b-fg
Copy link
Member

b-fg commented Jun 28, 2024

maybe we could have a plotter.jl file for this stuff and visualization scripts in src/, like a bin of unnecessary utils. Or call it nonutils.jl, even though this does not need to live on src/ but on ext/ which is triggered when loading Plots.jl.

@weymouth
Copy link
Collaborator

Yes, that's should not go in src, but in examples or ext.

@marinlauber
Copy link
Member Author

I have added the logger as an extension (via Plots). This means that the examples/TwoD_plots.jl file is now deprecated. I have modified examples/TwoD_circle.jl to show how this looks now. If we decide to go this route for the Plotting utils, I can do the same with Makie for 3D plots and update all the examples to work.

I also renamed the pressure logger example to examples/TwoD_circle_pressure_monitor.jl.

marinlauber pushed a commit to marinlauber/WaterLily that referenced this pull request Aug 12, 2024
@weymouth
Copy link
Collaborator

Reviving this PR. Now that we've move out examples, all that left is the logger.

Is this ready to go?

@b-fg
Copy link
Member

b-fg commented Sep 18, 2024

This looks good. I understand that the @debug macro does not compromise performance? And what about @logmsg?

@marinlauber
Copy link
Member Author

marinlauber commented Sep 26, 2024

Yes, none of the macros (@debug or @logmsg) should compromise performance when not used.
Maybe @logmsg will allow for a custom log level to be set and avoid triggering all the @debug of all the packages (that I have to filter). I will check this out.

@marinlauber
Copy link
Member Author

This is done now; let me know if anything remains to be done.
Are we happy with the Plots extensions as well?

@b-fg
Copy link
Member

b-fg commented Sep 26, 2024

Yes, I think it is much better to have all the plots utils in the extension.

@marinlauber
Copy link
Member Author

These are the results of the benchmark. If we are happy with this, I will merge it.

Benchmark environment: tgv sim_step! (max_steps=100)
▶ log2p = 6
┌─────────┬─────────────────┬────────┬───────────┬─────────────┬────────┬──────────┬──────────────────┬──────────┐
│ Backend │    WaterLily    │ Julia  │ Precision │ Allocations │ GC [%] │ Time [s] │ Cost [ns/DOF/dt] │ Speed-up │
├─────────┼─────────────────┼────────┼───────────┼─────────────┼────────┼──────────┼──────────────────┼──────────┤
│  CPUx04 │          master │ 1.10.5 │   Float32 │     2274514 │   0.00 │     2.52 │            95.96 │     1.00 │
│  CPUx04 │ pressure-solver │ 1.10.0 │   Float32 │     2274514 │   0.90 │     2.48 │            94.70 │     1.01 │
│  CPUx04 │ pressure-solver │ 1.10.5 │   Float32 │     2274514 │   0.00 │     2.49 │            94.91 │     1.01 │
│  CPUx16 │ pressure-solver │ 1.10.0 │   Float32 │     6137755 │   3.63 │     2.02 │            76.98 │     1.25 │
└─────────┴─────────────────┴────────┴───────────┴─────────────┴────────┴──────────┴──────────────────┴──────────┘
▶ log2p = 7
┌─────────┬─────────────────┬────────┬───────────┬─────────────┬────────┬──────────┬──────────────────┬──────────┐
│ Backend │    WaterLily    │ Julia  │ Precision │ Allocations │ GC [%] │ Time [s] │ Cost [ns/DOF/dt] │ Speed-up │
├─────────┼─────────────────┼────────┼───────────┼─────────────┼────────┼──────────┼──────────────────┼──────────┤
│  CPUx04 │          master │ 1.10.5 │   Float32 │     2021882 │   0.00 │    13.69 │            65.30 │     1.00 │
│  CPUx04 │ pressure-solver │ 1.10.0 │   Float32 │     2021882 │   0.16 │    13.59 │            64.81 │     1.01 │
│  CPUx04 │ pressure-solver │ 1.10.5 │   Float32 │     2021882 │   0.00 │    13.68 │            65.22 │     1.00 │
│  CPUx16 │ pressure-solver │ 1.10.0 │   Float32 │     5463590 │   0.54 │     9.16 │            43.67 │     1.50 │
└─────────┴─────────────────┴────────┴───────────┴─────────────┴────────┴──────────┴──────────────────┴──────────┘
Benchmark environment: jelly sim_step! (max_steps=100)
▶ log2p = 5
┌─────────┬─────────────────┬────────┬───────────┬─────────────┬────────┬──────────┬──────────────────┬──────────┐
│ Backend │    WaterLily    │ Julia  │ Precision │ Allocations │ GC [%] │ Time [s] │ Cost [ns/DOF/dt] │ Speed-up │
├─────────┼─────────────────┼────────┼───────────┼─────────────┼────────┼──────────┼──────────────────┼──────────┤
│  CPUx04 │          master │ 1.10.5 │   Float32 │     5545596 │   2.15 │     3.79 │           289.14 │     1.00 │
│  CPUx04 │ pressure-solver │ 1.10.0 │   Float32 │     5545596 │   0.68 │     3.60 │           274.94 │     1.05 │
│  CPUx04 │ pressure-solver │ 1.10.5 │   Float32 │     5545596 │   2.16 │     3.78 │           288.59 │     1.00 │
│  CPUx16 │ pressure-solver │ 1.10.0 │   Float32 │    14791162 │   2.86 │     3.00 │           228.56 │     1.27 │
└─────────┴─────────────────┴────────┴───────────┴─────────────┴────────┴──────────┴──────────────────┴──────────┘
▶ log2p = 6
┌─────────┬─────────────────┬────────┬───────────┬─────────────┬────────┬──────────┬──────────────────┬──────────┐
│ Backend │    WaterLily    │ Julia  │ Precision │ Allocations │ GC [%] │ Time [s] │ Cost [ns/DOF/dt] │ Speed-up │
├─────────┼─────────────────┼────────┼───────────┼─────────────┼────────┼──────────┼──────────────────┼──────────┤
│  CPUx04 │          master │ 1.10.5 │   Float32 │     6552597 │   0.57 │    14.71 │           140.24 │     1.00 │
│  CPUx04 │ pressure-solver │ 1.10.0 │   Float32 │     6552597 │   0.19 │    14.46 │           137.88 │     1.02 │
│  CPUx04 │ pressure-solver │ 1.10.5 │   Float32 │     6552597 │   0.57 │    14.72 │           140.39 │     1.00 │
│  CPUx16 │ pressure-solver │ 1.10.0 │   Float32 │    17568417 │   1.38 │     9.41 │            89.76 │     1.56 │
└─────────┴─────────────────┴────────┴───────────┴─────────────┴────────┴──────────┴──────────────────┴──────────┘

@b-fg
Copy link
Member

b-fg commented Oct 1, 2024

Comparison should be also on the CPUx16 backend right? Anyways this looks good. Do you have the chance to test on GPU, such as DelftBlue?

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