-
Notifications
You must be signed in to change notification settings - Fork 16
Tr/minor fixes #2347
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
base: main
Are you sure you want to change the base?
Tr/minor fixes #2347
Conversation
Also add `issubspace` for `FiniteDifferenceSpaces` The test currently fails because an error is not thrown for face spaces when out of bounds indexing.
`level` on a field on cell faces does not throw errors when attempting to access a level outside the space. This fixes that, but there may be a performance penalty.
unrolled_any( | ||
v -> subspace.local_geometry === level(all_local_geometry, v), | ||
1:nlevels(space), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that we want to unroll over all nlevels
for this. Isn't it sufficient to check the first level? When would it not be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the local geometry can vary spatially. For example:
julia> DataLayouts.level(Spaces.local_geometry_data(space), 1)[].coordinates
ZPoint(0.5)
julia> DataLayouts.level(Spaces.local_geometry_data(space), 3)[].coordinates
ZPoint(2.5)
julia> Fields.local_geometry_field(Fields.level(space, 1)).coordinates
ClimaCore.Geometry.ZPoint{Float64}-valued Field:
z: [0.5]
julia> Fields.local_geometry_field(Fields.level(space, 3)).coordinates
ClimaCore.Geometry.ZPoint{Float64}-valued Field:
z: [2.5]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I suppose it's safe to do the comparison over every point (even checking the range is still subject errors). But, we still probably don't want to unroll this call, since the result is not compile time known and nlevels
can be quite large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this method is not really needed. Its only use in src is to check broadcasting shapes, which already has unique methods for PointSpaces
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't think it's likely that we'll fall into such an error, but it is an extra layer of protection.
On the one hand, I don't think that this can be easily determined, with our current design, at compile-time. Ideally, we could add some sort of compile-time switches into our code that would allow / enforce these checks at runtime, and we could use it as a debugging feature. But it might be a hefty amount of work to make that feature available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure why I used unrolling here... do you think I should just remove this functionality, or leave it without unrolling? The only argument I have for keeping it is that if it exists for the extruded spectral element spaces, I'd expect it to also exist for finite difference spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the performance is likely fine without the unrolling, since the ===
comparison is likely pretty fast.
The only issue that I could see is that this operation is not compile-time known, and the logic of where this is called includes a check that the spaces are compatible (we error if they're not). If the check is not compile-time known, then it won't work inside a cuda kernel (this check is currently performed before the kernel is launched, but that won't be the case and it will need to work inside a cuda kernel with my columnwise!
design, so there may be clashes with that).
This PR addresses two issues:
field2array
is called with incompatible elements.level
function does not throwBoundsError
sFor the first issue, the message itself is corrected.
For the second issue, tests are added for
level(::Field)
.While writing these tests, I used
issubpace
, which was not definedfor
FiniteDifferenceFields
, so I added a method definition for that as well.Closes #2337
Closes #2320