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.

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

[next] Bug in embedded implementation of premap #1583

Open
egparedes opened this issue Jul 18, 2024 · 2 comments
Open

[next] Bug in embedded implementation of premap #1583

egparedes opened this issue Jul 18, 2024 · 2 comments
Assignees
Labels
triage: bug Something isn't working

Comments

@egparedes
Copy link
Contributor

egparedes commented Jul 18, 2024

This issue has been originally discovered by @nfarabullini . I'm only reporting it there since she is on vacation.

In a new Icon4py PR (https://github.com/C2SM/icon4py/pull/472/files) the field-vire embedded implementation of a premap operation crashes for unclear reasons since it was working previously for iterator embedded, therefore this is most likely a bug.

This is the definition of the operator crashing:

C2E2CODim = Dimension("C2E2CO", DimensionKind.LOCAL)
C2E2CO = FieldOffset("C2E2CO", source=CellDim, target=(CellDim, C2E2CODim))

@field_operator
def _compute_weighted_cell_neighbor_sum(
    field: Field[[CellDim, KDim], wpfloat],
    c_bln_avg: Field[[CellDim, C2E2CODim], wpfloat],
) -> Field[[CellDim, KDim], wpfloat]:
    field_avg = neighbor_sum(field(C2E2CO) * c_bln_avg, axis=C2E2CODim)
    return field_avg
@egparedes egparedes added the triage: bug Something isn't working label Jul 18, 2024
@egparedes egparedes self-assigned this Jul 18, 2024
@havogt
Copy link
Contributor

havogt commented Aug 2, 2024

Reading again the description of the implementation, it seems we overlooked the case X2X for a Field[Dims[X,Y],...]. In this case the domain of the connectivity is partially (but not fully contained in the domain of the field).

@havogt
Copy link
Contributor

havogt commented Aug 2, 2024

This line

if self.domain.dim_index(self.codomain) is None

is problematic.
It makes the above case a ALTER_STRUCT only. If we would implement the behavior that is described, we should raise an exception here. But that wouldn't solve the issue. Let's re-discuss how the above case fits in the categorization of connectivities.
Side-note: If I force ALTER_STRUCT | ALTER_DIMS on the connectivity the example works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants