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

Implement piecewise-linear rhod profile in condensation methods #1106

Merged
merged 46 commits into from
Aug 21, 2023

Conversation

mikhailmints
Copy link
Collaborator

@mikhailmints mikhailmints commented Jul 24, 2023

Closes #746, closes #765.
This implements a piecewise-linear rhod profile in the condensation methods instead of using rhod_mean. This change makes the condensation solver much less dependent on the timestep in the parcel environment, particularly for parcels with low velocities. For example, before the change, a particular run of the parcel model resulted in the following:
image
There was a very significant difference between the predicted supersaturation for different timestep values, which caused the solver to significantly overestimate the peak supersaturation and activation fraction unless the timestep was set to a very low value.
After the change, the results for different timesteps become nearly identical:
image
TODO: implement the same changes in the ThrustRTC backend.

@slayoo
Copy link
Member

slayoo commented Jul 24, 2023

Thank you, @mikhailmints !

@slayoo
Copy link
Member

slayoo commented Jul 24, 2023

Also, the plot you included seems a ready-to-use base for a new unit test! Asserting that the two lines match.
In many unit tests, we include a plot=False argument, which if turned to True, makes the test produce a plot (just before an assert) - so the plot can be included as well, just adding at the end:

        if plot:
            pyplot.show()
        else:
            pyplot.clf()

@claresinger
Copy link
Collaborator

Thank you @mikhailmints - looking great! Can you check for convergence with this new linear rhod profile with dz steps rather than dt steps? (Using dz rather than dt will remove the w dependence.) Maybe ranging from dz=0.01 to dz=10m? That would be helpful to know for the examples. You could add this as a plot/convergence test as a unit test, as Sylwester suggested above.

@trontrytel
Copy link
Contributor

Looks like macos-12, 3.7, smoke_tests/parcel and macos-12, 3.10, smoke_tests/parcel are timing out. Not sure what to do about them - could we split that chunk of tests into two to deal with the timing?

I'm bumping this PR, because I think it's a really beneficial update to the default behavior ;)

@slayoo
Copy link
Member

slayoo commented Aug 7, 2023

@trontrytel @mikhailmints @claresinger

Concerning the timeouts on macOS (which occur within SciPy code, not the PySDM solver), I'd say let's just increase the timeout.
However, I vote for waiting with merging till we have a test added that checks what's visible in the above plots - to make sure that the behaviour is asserted on CI, and that it will be checked for in future developments. @mikhailmints could you give a try at adding the new smoke test?

We have also the TODO of matching the changes in the ThrustRTC backend, but if well labelled and marked with a new issue, we can defer it for later.

Thanks!

@slayoo
Copy link
Member

slayoo commented Aug 17, 2023

The README parcel example is also a good test case here. If we run it for double the ascent altitude, there is some noise present, which disappears with the changes in this PR. Aiming at committing a README-based test case here soon.

@slayoo
Copy link
Member

slayoo commented Aug 17, 2023

closing and reopening to fix commit list

@slayoo slayoo closed this Aug 17, 2023
@slayoo slayoo reopened this Aug 17, 2023
… dqv_dt_pred are always 0 for Parcel env, while rhod is constant-in-time for kinematic simulations; mark volume as updated. Closes open-atmos#792
mikhailmints and others added 27 commits August 19, 2023 17:19
…aused by inconsistent density discretisation
… dqv_dt_pred are always 0 for Parcel env, while rhod is constant-in-time for kinematic simulations; mark volume as updated. Closes open-atmos#792
@slayoo slayoo merged commit 341de68 into open-atmos:main Aug 21, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants