Skip to content

Conversation

@tomchor
Copy link
Collaborator

@tomchor tomchor commented Sep 25, 2025

Resolves #4729

In addition to improving the behavior of nodes(), this PR, together with #4730, makes it possible to write different slices of a Field (or several Fields) into one NetCDF file.

@glwagner
Copy link
Member

glwagner commented Oct 1, 2025

Should nodes return a KernelFunctionOperation? If it did, would this PR be easier?

@tomchor
Copy link
Collaborator Author

tomchor commented Oct 6, 2025

Should nodes return a KernelFunctionOperation?

Not sure. I imagine this being more cumbersome for the end user, but possibly reducing code and makings things more flexible.

If it did, would this PR be easier?

A little, yes. But I don't think enough as to motivate this change by itself.

##### nodes
#####

nodes(f::Field; kwargs...) = nodes(f.grid, instantiated_location(f)...; indices=indices(f), kwargs...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
nodes(f::Field; kwargs...) = nodes(f.grid, instantiated_location(f)...; indices=indices(f), kwargs...)
nodes(f::Field; kwargs...) = nodes(f.grid, instantiated_location(f)...; indices=indices(f), kwargs...)

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 think I'm missing something; What's different here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

the extra line at the bottom of the file

Copy link
Member

Choose a reason for hiding this comment

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

he added whitespace below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, okay. Are we trying to always leave an extra line at the bottom of files?

@inline λnodes(grid::LLG, ℓx::C; with_halos=false) = _property(grid.λᶜᵃᵃ, ℓx, topology(grid, 1), grid.Nx, grid.Hx, with_halos)
@inline φnodes(grid::LLG, ℓy::F; with_halos=false) = _property(grid.φᵃᶠᵃ, ℓy, topology(grid, 2), grid.Ny, grid.Hy, with_halos)
@inline φnodes(grid::LLG, ℓy::C; with_halos=false) = _property(grid.φᵃᶜᵃ, ℓy, topology(grid, 2), grid.Ny, grid.Hy, with_halos)
@inline λnodes(grid::LLG, ℓx::F; with_halos=false, indices=Colon()) = getindex(_property(grid.λᶠᵃᵃ, ℓx, topology(grid, 1), grid.Nx, grid.Hx, with_halos), indices)
Copy link
Collaborator

@simone-silvestri simone-silvestri Oct 6, 2025

Choose a reason for hiding this comment

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

I guess this will not work on GPU right? Probably it will return a scalar indexing issue

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I think that's why GPU tests are failing. I'll try to come up with an alternative soon

@glwagner
Copy link
Member

glwagner commented Oct 6, 2025

After #4805, it will be pretty trivial to return KernelFunctionOperation for nodes. Do you want me to wait for this PR to do that, or should I do that before this PR so you don't have to worry about CPU vs GPU for nodes?

@glwagner
Copy link
Member

glwagner commented Oct 6, 2025

Basically if nodes return KernelFunctionOperation, then the way to get correct indices will be the same as building a Field and passing indices to the Field constructor. We won't need any "special" functionality for windowing the nodes, compared to windowing Field.

@tomchor
Copy link
Collaborator Author

tomchor commented Oct 6, 2025

After #4805, it will be pretty trivial to return KernelFunctionOperation for nodes. Do you want me to wait for this PR to do that, or should I do that before this PR so you don't have to worry about CPU vs GPU for nodes?

You can go ahead and make nodes return KernelFunctionOperation first!

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.

nodes() returns the full grid when given a smaller view of a Field

5 participants