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

Fix CI (0d field slice) #383

Merged
merged 6 commits into from
Feb 16, 2024
Merged

Fix CI (0d field slice) #383

merged 6 commits into from
Feb 16, 2024

Conversation

edopao
Copy link
Contributor

@edopao edopao commented Feb 12, 2024

The latest commit on gt4py introduces a syntax feature: if you slice a field with full indexing, the result is still a field, a 0d field specifically. In order to use it as a scalar, the DSL program has to call .as_scalar(). Before this gt4py commit, the result of slicing would have been a scalar.

This PR adopts the new gt4py syntax feature. In order to do this, we bump the version of gt4py Spack package in stable CI job to 1.0.3.1.

@edopao edopao changed the title Fix CI (non-backwards compatible change on gt4py main) Fix CI (0d field slice) Feb 12, 2024
Expected argument 'elevp1' to be of type 'int32', got 'int64'.
@edopao edopao marked this pull request as ready for review February 13, 2024 08:44
@edopao edopao requested a review from havogt February 13, 2024 08:44
@havogt havogt requested review from halungge and muellch and removed request for havogt February 13, 2024 09:47
@muellch
Copy link
Contributor

muellch commented Feb 13, 2024

I would just need some context, what is the error if this fix is not included?

@edopao
Copy link
Contributor Author

edopao commented Feb 13, 2024

The latest commit on gt4py introduces a syntax feature. The feature is that if you slice a field with full indexing, the result is still a field, a 0d field specifically. In order to use it as a scalar, the DSL program has to call .as_scalar(). Before this gt4py commit, the result would have been a scalar.

The question here is what do we mean by stable gt4py version, in the Spack CI job, and if we should provide backwards-compatibility checks like the one proposed in this PR. An alternative to the change proposed in this PR is to use the new syntax (which requires calling .as_scalar()) and bump the revision of stable gt4py.

@halungge
Copy link
Contributor

The latest commit on gt4py introduces a syntax feature. The feature is that if you slice a field with full indexing, the result is still a field, a 0d field specifically. In order to use it as a scalar, the DSL program has to call .as_scalar(). Before this gt4py commit, the result would have been a scalar.

The question here is what do we mean by stable gt4py version, in the Spack CI job, and if we should provide backwards-compatibility checks like the one proposed in this PR. An alternative to the change proposed in this PR is to use the new syntax (which requires calling .as_scalar()) and bump the revision of stable gt4py.

I would opt for the latter, as long as nobody relies on stable.

@edopao
Copy link
Contributor Author

edopao commented Feb 13, 2024

The latest commit on gt4py introduces a syntax feature. The feature is that if you slice a field with full indexing, the result is still a field, a 0d field specifically. In order to use it as a scalar, the DSL program has to call .as_scalar(). Before this gt4py commit, the result would have been a scalar.
The question here is what do we mean by stable gt4py version, in the Spack CI job, and if we should provide backwards-compatibility checks like the one proposed in this PR. An alternative to the change proposed in this PR is to use the new syntax (which requires calling .as_scalar()) and bump the revision of stable gt4py.

I would opt for the latter, as long as nobody relies on stable.

That's OK. In this case, @havogt should we make a gt4py v1.0.4 release or should I make a custom icon4py tag?

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.

Copy link
Contributor

@muellch muellch left a comment

Choose a reason for hiding this comment

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

Considering the circumstances, looks good to me.

I do not agree with a field of size 1 not decaying into a scalar, but that is another matter.

@edopao
Copy link
Contributor Author

edopao commented Feb 16, 2024

cscs-ci run default

@edopao
Copy link
Contributor Author

edopao commented Feb 16, 2024

launch jenkins spack

@edopao edopao merged commit fe3e745 into main Feb 16, 2024
5 checks passed
@edopao edopao deleted the fix-0d_field_slice branch February 16, 2024 11:01
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.

4 participants