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

Feature/bmauer/fixes #2149 #2196

Merged
merged 6 commits into from
Jun 21, 2023
Merged

Feature/bmauer/fixes #2149 #2196

merged 6 commits into from
Jun 21, 2023

Conversation

bena-nasa
Copy link
Collaborator

@bena-nasa bena-nasa commented Jun 16, 2023

This PR updates the arithemetic expression parser to use the new field utilities to work ons fields, rather than arrays. This means it can now work in a rank/type agnostic fashion which crucial for future development since ExtDataDriver.x makes use of this extensively and was limited to 2D/3D gridded fields without this.

In addition I found several bugs in the new field utilities. These were:

  1. The binary operator template had a bug, and the test was not working to catch it which I quickly found when trying to use these in the expression parser.
  2. The field clone function had been improperly implemented in the initial fieldBlas implementation and moreover the test that had been added for it that wasn't actually added with an @test nor even if it had been was it doing anything. This function was implemented and a proper test was added.
  3. The initial testing framework in the geom directory never even added a way to create and test fields with ungridded dimensions for some bizarre reason. I added that.

This also requires 2 new features to the field geom class:

  1. I added a "**" operator to the binary operators as this was necessary for the expression parser.

  2. A "broadcast" copy, the idea is if you want to copy field x of rank x_r to field y of rank y_r, if y_r = x_r + 1 and the shape of the first x_r dimensions in field y matches field x, then copy the pointer from x to y and replicate x along the last dimension of y. This is 110% crucial for ExtDatDriver.x and without this I would be hosed and this is non-negotiable for inclusion.

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Trivial change (affects only documentation or cleanup)

Checklist:

  • I have tested this change with a run of GEOSgcm (if non-trivial)
  • I have added one of the required labels (0 diff, 0 diff trivial, 0 diff structural, non 0-diff)
  • I have updated the CHANGELOG.md accordingly following the style of Keep a Changelog

@bena-nasa bena-nasa added the 🎁 New Feature This is a new feature label Jun 16, 2023
@bena-nasa bena-nasa requested review from a team as code owners June 16, 2023 19:20
@bena-nasa bena-nasa added the 0 Diff The changes in this pull request have verified to be zero-diff with the target branch. label Jun 16, 2023
@bena-nasa bena-nasa requested review from tclune and mathomp4 June 16, 2023 20:04
@bena-nasa
Copy link
Collaborator Author

Yes I see the gfortran test is failing. I will see what is going on and fix.

@bena-nasa
Copy link
Collaborator Author

ok, fixed gnu on Discover, lets see what the CI does.

Copy link
Contributor

@atrayano atrayano left a comment

Choose a reason for hiding this comment

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

Looks reasonable

@bena-nasa bena-nasa merged commit 88233a0 into develop Jun 21, 2023
@bena-nasa bena-nasa deleted the feature/bmauer/fixes-#2149 branch June 21, 2023 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 Diff The changes in this pull request have verified to be zero-diff with the target branch. 🎁 New Feature This is a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants