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

Advection-diffusion of passive tracer using a turbulent flow #53

Merged
merged 78 commits into from
Jun 7, 2022

Conversation

jbisits
Copy link
Collaborator

@jbisits jbisits commented May 31, 2022

This PR adds the ability to advect-diffuse a passive tracer using a flow generated by the MultiLayerQG module from the GeophysicalFlows.jl package with a constant diffusivity. To implement this another method has been added to TracerAdvectionDiffusion.Problem that takes in a MultiLayerQG.Problem as an argument. The TracerAdvectionDiffusion.Problem is then setup on the same size grid and number of layers as the MultiLayerQG.Problem. Additional arguments are \kappa and \eta, the constant diffusivity, the stepper and the tracer_release. The tracer_release argument allows the user to specify some length of time to run the flow for (e.g. so the flow reaches a statistical equilibrium) prior to releasing the tracer into the flow.

I have written an example ,turbulent_advection-diffusion.jl, showing how the module works. In doing this I have added Plots.jl to the Project.toml which I did not want to do (nor does it need to be there). I have no experience setting up the example to be run and rendered into a GitHub page so this is why it has occured.

I have not implemented it yet but as a test for the function I will set the MultilayerQG.Problem to produce noflow (i.e. flow fields are 0 for the whole simulation) and check the diffusion of a tw-dimensional Gaussian in a similar manner to one of the tests that is already in the test_traceradvectiondiffusion.jl file.

I am not sure if this is the best way to implement this functionality into the PassiveTracerFlows.jl package but I have used it a lot over the last year or so and it has been very reliable and reasonably quick even on the much larger domains I was using.

@jbisits jbisits requested a review from navidcy May 31, 2022 00:19
jbisits and others added 8 commits May 31, 2022 17:51
@navidcy
Copy link
Member

navidcy commented Jun 1, 2022

@jbisits I made some changes:

  • dropped Plots from package depot
  • added your example in the literated list
  • use latest FourierFlows and GeophysicalFlows version
  • updated syntax to work for any number of layers

@navidcy navidcy added the enhancement New feature or request label Jun 1, 2022
Copy link
Member

@navidcy navidcy left a comment

Choose a reason for hiding this comment

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

made a few remarks
I will have a closer look over the weekend!

navidcy and others added 7 commits June 1, 2022 14:50
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
@jbisits
Copy link
Collaborator Author

jbisits commented Jun 1, 2022

@navidcy thanks for making the changes above, all of the package organisation is new to me!

@navidcy
Copy link
Member

navidcy commented Jun 7, 2022

taking the absolute value of the tracer doesn't seem right... :(
let me try to heal the instabilities

@navidcy
Copy link
Member

navidcy commented Jun 7, 2022

  stepforward!(ADprob)
  stepforward!(params.MQGprob)
  MultiLayerQG.updatevars!(params.MQGprob)

the MultiLayerQG.updatevars!(params.MQGprob) was missing... thus the advecting velocities were not updated...

This seems to have fixed it. Have a look:

turbulentflow_advection-diffusion.mp4

The proper solution would be to define an abstract type of a CoupledProblem that would include both the PassiveTracerFlows.Problem and the MultiLayerQG.Problem and then define a new method for stepforward! for this coupled problem, e.g.,

function stepforward!(coupled_prob::CoupledProblem)
  stepforward!(coupled_prob.ADprob)
  stepforward!(coupled_prob.MQGprob)
  MultiLayerQG.updatevars!(coupled_prob.MQGprob)
end

But we can defer this for a future PR?

@navidcy navidcy self-requested a review June 7, 2022 20:08
@navidcy
Copy link
Member

navidcy commented Jun 7, 2022

@jbisits feel free to merge when tests pass if you don't have anything else to add.

Sorry it took so long.

@navidcy
Copy link
Member

navidcy commented Jun 7, 2022

oh I guess you probably don't have the right to merge? I'll do it ;)

@jbisits
Copy link
Collaborator Author

jbisits commented Jun 7, 2022

taking the absolute value of the tracer doesn't seem right... :( let me try to heal the instabilities

Yeah taking absolute value was something we did on larger domains at the start of simulations where some values were really small and negative.

@jbisits
Copy link
Collaborator Author

jbisits commented Jun 7, 2022

  stepforward!(ADprob)
  stepforward!(params.MQGprob)
  MultiLayerQG.updatevars!(params.MQGprob)

the MultiLayerQG.updatevars!(params.MQGprob) was missing... thus the advecting velocities were not updated...

This seems to have fixed it. Have a look:

turbulentflow_advection-diffusion.mp4

This is great!!

The proper solution would be to define an abstract type of a CoupledProblem that would include both the PassiveTracerFlows.Problem and the MultiLayerQG.Problem and then define a new method for stepforward! for this coupled problem, e.g.,

function stepforward!(coupled_prob::CoupledProblem)
  stepforward!(coupled_prob.ADprob)
  stepforward!(coupled_prob.MQGprob)
  MultiLayerQG.updatevars!(coupled_prob.MQGprob)
end

But we can defer this for a future PR?

Agreed.

@jbisits
Copy link
Collaborator Author

jbisits commented Jun 7, 2022

@jbisits feel free to merge when tests pass if you don't have anything else to add.

Sorry it took so long.

Did not take too long! Thanks for all your help!!!

@jbisits
Copy link
Collaborator Author

jbisits commented Jun 7, 2022

I can have a look at some of these tests in a bit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants