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

Tracer density #434

Merged
merged 8 commits into from
Sep 12, 2023
Merged

Tracer density #434

merged 8 commits into from
Sep 12, 2023

Conversation

ta440
Copy link
Collaborator

@ta440 ta440 commented Sep 1, 2023

Added a tracer density diagnostic. This is useful when examining conservation of mass when transporting species via the dry density and mixing ratio.
Here are examples of plotting the normalised difference in the tracer density diagnostic over time, using the planar_nair_gaussian case as examined the Bendall, 'Trilemma' paper.
tracer_density_consistency
tracer_density_convergence


name = "TracerDensity"

def __init__(self, m_X, rho_d):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this __init__ method should still have the arguments of the parent class, because the user might still want to specify the function space and the computation method.

We also need to make clear whether the m_X and rho_d arguments should be strings (the names of the fields) or the Function variables. I think if we change the name of the arguments to mixing_ratio_name and density_name then it is clearer?

So I suggest something like:

def __init__(self, mixing_ratio, density_name, space=None, method='interpolate', required_fields=()):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with having more descriptive variable names.
Is the required_fields=() just to initialise this variable before using it in:
super().init(method='interpolate', required_fields=(mixing_ratio_name, density_name))
?

I've tested the four different methods with this diagnostic. It works when specifying 'interpolate' or 'project'. However, there are issues when specifying 'assign':
ValueError: All functions in the expression must have the same element as the assignee
and 'solve':
NotImplementedError: Solve method has not been implemented for diagnostic TracerDensity

Should this diagnostic be able to be used with these methods, or is it to be expected that these aren't compatible?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I think you're right, we don't need required_fields in the __init__, as this should just be set in the call to super().__init__.

For methods, 'assign' only makes sense when the density and the mixing ratio field in the same space (what assign does is evaluate the values at the degrees of freedom). The solve method is there if we need to solve a more complicated equation to get a diagnostic, rather than interpolating/projecting/assigning. In this case I don't think we need a solve method so it should throw an error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, thanks.
I'll put in the function header that:
Valid options are 'interpolate', 'project' and 'assign', so that one doesn't try to use 'solve' with this

def __init__(self, m_X, rho_d):
"""
Args:
m_X: the mixing ratio of the tracer
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to include the type of the argument in the docstrings, so this should be something like:

mixing_ratio_name (str), the name of the tracer mixing ratio variable.

and so on for the other variables

rho_d = state_fields(self.rho_d)
space = m_X.function_space()
self.expr = m_X*rho_d
super().setup(domain, state_fields, space=space)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we include space as an argument to the __init__ then we won't need to include it here

Copy link
Contributor

@tommbendall tommbendall left a comment

Choose a reason for hiding this comment

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

This all looks good to me, thanks Tim!

@tommbendall tommbendall merged commit 1a377a1 into main Sep 12, 2023
4 checks passed
@tommbendall tommbendall deleted the tracer_density branch September 12, 2023 07:04
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