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.

Sign up for GitHub

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

Jump to bottom

Enable DSS for Slab Models #387

Merged
merged 1 commit into from
Aug 18, 2023
Merged

Enable DSS for Slab Models #387

merged 1 commit into from
Aug 18, 2023

Conversation

LenkaNovak
Copy link
Collaborator

@LenkaNovak LenkaNovak commented Aug 10, 2023 •
edited
Loading

Purpose

Closes #381 (see for details and QA assessment)

  • I have read and checked the items on the review checklist.

@LenkaNovak LenkaNovak marked this pull request as ready for review August 10, 2023 19:45
akshaysridhar
akshaysridhar reviewed Aug 11, 2023
View reviewed changes
experiments/AMIP/modular/components/slab_utils.jl
for key in propertynames(Y)
field = getproperty(Y, key)
buffer = get_dss_buffer(axes(field), p)
ClimaCore.Spaces.weighted_dss!(field, buffer)
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

Choose a reason for hiding this comment

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

If we are calling weighted_dss internally, I would recommend prefixing weighted to this specialized function name!

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we are passing argument t if we are not using it inside the function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good, thanks (see comment above)

Copy link
Member

@sriharshakandala sriharshakandala Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If we are actually calling weighted_dss function inside, can we also clarify this in the calling function name! (e.g. weighted_dss_slab! or so. We currently support both pure dss and weighted_dss (which combines the effect of M^-1 x dss)in ClimaCore.jl and it would be good to disambiguate!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good, changed to weighted_dss_slab! and merging. Thank you both!

@sriharshakandala
Copy link
Member

sriharshakandala commented Aug 17, 2023 •
edited
Loading

As discussed, since the primary objective of this PR is to add the dss buffer, I am ok with renaming the function and addressing the unused third argument issue a bit later since it is entangled with CliMATimeSteppers.jl.

Generally speaking, the application of DSS is a part of the RHS /tendency function computation! However, it seems to be isolated and treated differently than usual, which is probably the reason this ambiguity arises in the Time Stepper here!

@LenkaNovak
init
20f5fe7 
fix FT

reinit all sims

FT fix

sea ice fix

clean slab utils

revs

revs
@akshaysridhar akshaysridhar self-requested a review August 17, 2023 23:39
@LenkaNovak
Copy link
Collaborator Author

As discussed, since the primary objective of this PR is to add the dss buffer, I am ok with renaming the function and addressing the unused third argument issue a bit later since it is entangled with CliMATimeSteppers.jl.

Generally speaking, the application of DSS is a part of the RHS /tendency function computation! However, it seems to be isolated and treated differently than usual, which is probably the reason this ambiguity arises in the Time Stepper here!

That's how I understand it too. Here it is called as a callback - I guess some models (and their tendencies) could be pointwise and be independent of ClimaCore. The coupler could then use the ClimaCore Field objects to apply the models spatially, so in this case we'd definitely need a callback. In the slab ocean / Bucket cases, maybe either method works (as long as it's ok for the dss to be applied only once and after the stepping, rather during each substep)?

@LenkaNovak LenkaNovak mentioned this pull request Aug 18, 2023
Open
@LenkaNovak
Copy link
Collaborator Author

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 18, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 06a9bfc into main Aug 18, 2023
10 checks passed
@bors bors bot deleted the ln/add-dss-to-slabs branch August 18, 2023 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akshaysridhar akshaysridhar akshaysridhar approved these changes

@sriharshakandala sriharshakandala sriharshakandala approved these changes

@kmdeck kmdeck Awaiting requested review from kmdeck

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Apply dss to slab model Y+dY
3 participants
@LenkaNovak @sriharshakandala @akshaysridhar