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

Port wgtfac_c #365

Merged
merged 16 commits into from
Feb 13, 2024
Merged

Port wgtfac_c #365

merged 16 commits into from
Feb 13, 2024

Conversation

jonasjucker
Copy link
Contributor

@jonasjucker jonasjucker commented Jan 24, 2024

Port stencil from src/atm_dyn_iconam/mo_vertical_grid.f90.

       DO jk = 2, nlev
          p_nh(jg)%metrics%wgtfac_c(1:nlen,jk,jb) =  &
           (p_nh(jg)%metrics%z_ifc(1:nlen,jk-1,jb) - &
            p_nh(jg)%metrics%z_ifc(1:nlen,jk,jb)) /  &
           (p_nh(jg)%metrics%z_ifc(1:nlen,jk-1,jb) - &
            p_nh(jg)%metrics%z_ifc(1:nlen,jk+1,jb))
        ENDDO

It does not include the edge-cases k=1 and k=nlevp1
What to do about that?

@jonasjucker jonasjucker changed the base branch from main to port_nudgecoeff_e January 24, 2024 14:12
@jonasjucker jonasjucker changed the base branch from port_nudgecoeff_e to main January 24, 2024 14:12
@jonasjucker
Copy link
Contributor Author

launch jenkins spack

@jonasjucker
Copy link
Contributor Author

cscs-ci run default

@jonasjucker jonasjucker requested a review from halungge January 29, 2024 08:24
@jonasjucker jonasjucker changed the base branch from main to port_nudgecoeff_e January 29, 2024 08:26
@jonasjucker jonasjucker changed the base branch from port_nudgecoeff_e to main January 29, 2024 08:26
@jonasjucker jonasjucker changed the base branch from main to port_nudgecoeff_e January 29, 2024 08:28
@halungge
Copy link
Contributor

Port stencil from src/atm_dyn_iconam/mo_vertical_grid.f90.

       DO jk = 2, nlev
          p_nh(jg)%metrics%wgtfac_c(1:nlen,jk,jb) =  &
           (p_nh(jg)%metrics%z_ifc(1:nlen,jk-1,jb) - &
            p_nh(jg)%metrics%z_ifc(1:nlen,jk,jb)) /  &
           (p_nh(jg)%metrics%z_ifc(1:nlen,jk-1,jb) - &
            p_nh(jg)%metrics%z_ifc(1:nlen,jk+1,jb))
        ENDDO

It does not include the edge-cases k=1 and k=nlevp1 What to do about that?

Port stencil from src/atm_dyn_iconam/mo_vertical_grid.f90.

       DO jk = 2, nlev
          p_nh(jg)%metrics%wgtfac_c(1:nlen,jk,jb) =  &
           (p_nh(jg)%metrics%z_ifc(1:nlen,jk-1,jb) - &
            p_nh(jg)%metrics%z_ifc(1:nlen,jk,jb)) /  &
           (p_nh(jg)%metrics%z_ifc(1:nlen,jk-1,jb) - &
            p_nh(jg)%metrics%z_ifc(1:nlen,jk+1,jb))
        ENDDO

It does not include the edge-cases k=1 and k=nlevp1 What to do about that?

I assume the originally initalize with 0 everywhere and then overwrite the "inner" levels. That is what usually happens in ICON. Eventually we will then do the same thing. Allocate with the right size and 0 and overwrite the inner levels by setting the vertical_start, vertical_end correctly. Exactly as you do in the verification test.

Copy link
Contributor

@halungge halungge left a comment

Choose a reason for hiding this comment

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

Looks good, only some small clean ups in the test.

from icon4py.model.common.dimension import CellDim, KDim
from icon4py.model.common.grid.horizontal import HorizontalMarkerIndex
from icon4py.model.common.interpolation.stencils.calc_wgtfac_c import calc_wgtfac_c
from icon4py.model.common.test_utils.datatest_fixtures import ( # noqa: F401 # import fixtures from test_utils package
Copy link
Contributor

Choose a reason for hiding this comment

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

If you have several test files in this folder you might want to put the fixture imports into a conftest.py pytest checks that one automatically for fixtures and makes them available to the tests. conftest.py would essentially just look like this:

from icon4py.model.common.test_utils.datatest_fixtures import (  # noqa: F401
    damping_height,
    data_provider,
    datapath,
    decomposition_info,
    download_ser_data,
    experiment,
    grid_savepoint,
    icon_grid,
    interpolation_savepoint,
    processor_props,
    ranked_data_path,
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this info.

I would keep that for the next PR that add another stencil to metrics.
For now I think it is fine as is

@jonasjucker
Copy link
Contributor Author

The thing with the edge-case is that the field is filled for these indices as well, just below the loop:

          p_nh(jg)%metrics%wgtfac_c(1:nlen,1,jb) = &
           (p_nh(jg)%metrics%z_ifc(1:nlen,2,jb) -  &
            p_nh(jg)%metrics%z_ifc(1:nlen,1,jb)) / &
           (p_nh(jg)%metrics%z_ifc(1:nlen,3,jb) -  &
            p_nh(jg)%metrics%z_ifc(1:nlen,1,jb))
          p_nh(jg)%metrics%wgtfac_c(1:nlen,nlevp1,jb) = &
           (p_nh(jg)%metrics%z_ifc(1:nlen,nlev,jb) -    &
            p_nh(jg)%metrics%z_ifc(1:nlen,nlevp1,jb)) / &
           (p_nh(jg)%metrics%z_ifc(1:nlen,nlev-1,jb) -  &
            p_nh(jg)%metrics%z_ifc(1:nlen,nlevp1,jb))

So the current implementation of this stencil does not return the correct field.

Is this correct that way?
How do you plan to solve the edge-cases then?

Copy link

Mandatory Tests

Please make sure you run these tests via comment before you merge!

  • cscs-ci run default
  • launch jenkins spack

Optional Tests

To run benchmarks you can use:

  • cscs-ci run benchmark

In case your change might affect downstream icon-exclaim, please consider running

  • launch jenkins icon

For more detailed information please look at CI in the EXCLAIM universe.

@jonasjucker
Copy link
Contributor Author

launch jenkins spack

@jonasjucker
Copy link
Contributor Author

cscs-ci run default

@jonasjucker jonasjucker merged commit 654e1de into port_nudgecoeff_e Feb 13, 2024
1 of 2 checks passed
@jonasjucker jonasjucker deleted the port_wgtfac_c branch February 13, 2024 11:44
jonasjucker added a commit that referenced this pull request Feb 19, 2024
* add nudgecoeffs_e

* add test

* test parses

* remove horizontal bounds

* stencil runs

* second implementation of stencil

* cleanup stencil

* correct indexing for end_index and refin_ctrl

* remove unused import

* import all fixtures, format

* hardcode grf_nudge_start_e to 10

* translate index to Fortran value

* Introduce RefinCtrlLevel

* formatting

* add dot

* review suggestions

* Port wgtfac_c (#365)

* cleanup

* formatting

* rename stencils, review suggestions

* formatting

---------

Co-authored-by: Jonas Jucker <jonas.jucker@c2sm.ethz.ch>
OngChia pushed a commit that referenced this pull request Jan 14, 2025
* add nudgecoeffs_e

* add test

* test parses

* remove horizontal bounds

* stencil runs

* second implementation of stencil

* cleanup stencil

* correct indexing for end_index and refin_ctrl

* remove unused import

* import all fixtures, format

* hardcode grf_nudge_start_e to 10

* translate index to Fortran value

* Introduce RefinCtrlLevel

* formatting

* add dot

* review suggestions

* Port wgtfac_c (#365)

* cleanup

* formatting

* rename stencils, review suggestions

* formatting

---------

Co-authored-by: Jonas Jucker <jonas.jucker@c2sm.ethz.ch>
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.

2 participants