Skip to content

Properly extend default_included_properties(::SeaIceModel)#107

Merged
navidcy merged 3 commits intomainfrom
ncc/extend
Jan 22, 2026
Merged

Properly extend default_included_properties(::SeaIceModel)#107
navidcy merged 3 commits intomainfrom
ncc/extend

Conversation

@navidcy
Copy link
Copy Markdown
Member

@navidcy navidcy commented Jan 22, 2026

No description provided.


import Oceananigans.Architectures: architecture
import Oceananigans.Models: update_model_field_time_series!
import Oceananigans.OutputWriters: default_included_properties
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this was missing... so the method was overwriting the other methods by Oceananigans?

cc @giordano

Copy link
Copy Markdown

@giordano giordano Jan 22, 2026

Choose a reason for hiding this comment

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

It was just another function with the same name, that's usually called "shadowing".

This is the reason why I really don't like the import way of extending functions: you rely on a statement which is far away, and if that's, for example, moved during refactoring...you may never notice it. The Module.function_name(...) =... syntax is perhaps more verbose, but it's (1) local and (2) more explicit about the intent ("look, here I'm extending a function defined in another module"), so yeah, verbosity is on purpose. If with this syntax that function doesn't exist in the other module, you get a loud error during precompilation, instead of silently doing the wrong thing when using import for extending the function.

@navidcy navidcy merged commit 5d5ae5f into main Jan 22, 2026
7 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

Development

Successfully merging this pull request may close these issues.

3 participants