Skip to content

Expose include_rivers_and_icebergs kwarg instead of using the default #594

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

Merged
merged 1 commit into from
Aug 6, 2025

Conversation

navidcy
Copy link
Member

@navidcy navidcy commented Aug 1, 2025

Despite include_rivers_and_icebergs = false is the default, I think it's good idea to expose it so that it's obvious to anybody that they need to tweak this to include runoff and icebergs.

We might want to consider making include_rivers_and_icebergs = true the default at some point.

@navidcy navidcy added the atmosphere-ocean coupling ⛅️ 🌊 When the relationship isn't as smooth as expected label Aug 1, 2025
@glwagner
Copy link
Member

glwagner commented Aug 1, 2025

We might want to consider making include_rivers_and_icebergs = true the default at some point.

Does it change the solution? The fluxes are incredibly weak.

@navidcy
Copy link
Member Author

navidcy commented Aug 1, 2025

We might want to consider making include_rivers_and_icebergs = true the default at some point.

Does it change the solution? The fluxes are incredibly weak.

I haven't quantified the effect. There should be an effect near rivers. But their effect might not be apparent in short simulations. Given that I don't know atm I am also not suggesting we change the defaults.

This PR only means to expose to the users the keyword argument since one could be thinking that JRA55PrescribedAtmosphere by default would uses all the JRA55-do fields.

@taimoorsohail
Copy link
Collaborator

I think the only one that makes a substantial difference in the Amazon discharge, about 1 Sv. It's a visible signal in basin-scale integrals (zonal integrals of freshwater content in the Atlantic, for example). For this reason I think we should include it down the track...

@glwagner
Copy link
Member

glwagner commented Aug 6, 2025

It's worth noting that we wouild also like ot move this to a "land" component for OceanSeaIceModel. These are properly understood as fluxes form a land component, not from an atmosphere component.

@navidcy
Copy link
Member Author

navidcy commented Aug 6, 2025

It's worth noting that we wouild also like ot move this to a "land" component for OceanSeaIceModel. These are properly understood as fluxes form a land component, not from an atmosphere component.

That's a good point. These are not atmospheric-related fields...
I think the river and icebergs are not originally part of JRA55 reanalysis; they are only included in the JRE55-do (=drive ocean) version.

I'm merging since this PR doesn't change anything other than making the kwarg more visible in the examples.

@navidcy navidcy merged commit 39975d1 into main Aug 6, 2025
17 checks passed
@navidcy navidcy deleted the ncc-ts/runoff branch August 6, 2025 23:35
@glwagner
Copy link
Member

glwagner commented Aug 7, 2025

Maybe we shouldn't include in the examples though right? What's the point?

@navidcy
Copy link
Member Author

navidcy commented Aug 7, 2025

It was hidden. Until it’s in a land model or somewhere else this is the way to include the river runoff and it was quite hidden.

I didn’t add anything. I just made the kwarg explicitly use false instead of relying to defaults.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
atmosphere-ocean coupling ⛅️ 🌊 When the relationship isn't as smooth as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants