Ability to generate planar metric terms and addition of `test_eta.py` unit test by fmalatino · Pull Request #65 · NOAA-GFDL/NDSL

fmalatino · 2024-08-20T19:19:14Z

Description
A conditional statement has been added to the __init__ method of the MetricTerms class to allow for generating surface level metric terms (no hybrid pressure values). Initialization of such a MetricTerms object will now contain zero-valued hybrid pressures.

The unit test test_eta.py from pace/tests/main/grid has now been moved into ndsl/tests/grid along with a python module for generating the test eta level containing files.

This PR must be reviewed and merged after PR 92 is merged in pace

How Has This Been Tested?
Tested via the tests contained within the tests directory of NDSL, and in the tests of the dependent repositories

Pace tests will fail as the changes from PR 92 in Pace are necessary for passing.

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules
New check tests, if applicable, are included

…tion for surface level (no eta_file specified)

fmalatino · 2024-08-22T14:56:29Z

Given that metric terms, and the grid generation methods as a whole are a part of NDSL, does it make sense to move the unit test tests/main/grid/test_eta.py in Pace to NDSL?

FlorianDeconinck · 2024-08-26T19:52:01Z

Given that metric terms, and the grid generation methods as a whole are a part of NDSL, does it make sense to move the unit test tests/main/grid/test_eta.py in Pace to NDSL?

Yes, please pull it down. We decided that the FV grid would come preset with NDSL, let's commit fully to it.

…ed eta_file generator script for testing

…re/no_eta

oelbert

Minor documentation changes mostly, otherwise lgtm

oelbert · 2024-08-29T19:42:00Z

+    """This test checks to see that the program
+    fails when (1) the eta_file is not specified in the yaml
+    configuration file; and (2), the computed eta values
+    increase non-monotonically.  For the latter test, the eta_file
+    is specified in test_config_not_mono.yaml file and
+    the ak and bk values in the eta_file have been changed nonsensically
+    to result in erronenous eta values.


It looks like this test only does the first of these two, so the docstring should reflect that

This docstring still needs to be updated

oelbert · 2024-08-29T19:44:40Z

+    """This test checks to see that the program
+    fails when (1) the eta_file is not specified in the yaml
+    configuration file; and (2), the computed eta values
+    increase non-monotonically.  For the latter test, the eta_file
+    is specified in test_config_not_mono.yaml file and
+    the ak and bk values in the eta_file have been changed nonsensically
+    to result in erronenous eta values.
+    """


Same as above, please update the docstring to reflect the test

same with this one

Co-authored-by: Oliver Elbert <oliver.elbert36@gmail.com>

oelbert · 2024-09-04T21:07:38Z

+    """This test checks to see that the program
+    fails when (1) the eta_file is not specified in the yaml
+    configuration file; and (2), the computed eta values
+    increase non-monotonically.  For the latter test, the eta_file
+    is specified in test_config_not_mono.yaml file and
+    the ak and bk values in the eta_file have been changed nonsensically
+    to result in erronenous eta values.


This docstring still needs to be updated

oelbert · 2024-09-04T21:07:52Z

+    """This test checks to see that the program
+    fails when (1) the eta_file is not specified in the yaml
+    configuration file; and (2), the computed eta values
+    increase non-monotonically.  For the latter test, the eta_file
+    is specified in test_config_not_mono.yaml file and
+    the ak and bk values in the eta_file have been changed nonsensically
+    to result in erronenous eta values.
+    """


same with this one

oelbert · 2024-09-04T21:11:21Z

+                self._ak,
+                self._bk,
+            ) = self._set_hybrid_pressure_coefficients(eta_file, ak, bk)
+        else:


Is this the behavior we want? I thought we'd rather raise a ValueError or something here?

A ValueError is raised deeper in when the file specified is either non-existent, and even lower if the values do not fit the size passed in the config. My thought was that we could still have hybrid pressure values; zero valued if no file passed, or specified when passed.

… eta_file variable in generation.py to use an empty string in conditional statement for hybrid pressure generation

FlorianDeconinck

Cleanup the default eta_file using the pythonic None

FlorianDeconinck

Fix a type hint and we are ready to go

… aspect

FlorianDeconinck

LGTM

oelbert

👍

Added conditional to MetricTerms init to allow for metric term genera…

1bda968

…tion for surface level (no eta_file specified)

fmalatino marked this pull request as draft August 20, 2024 19:37

Changes to support testing in pace for eta file specification

b18880b

fmalatino mentioned this pull request Aug 22, 2024

pace/tests/main/grid/test_eta.py moved to NDSL/tests/grid/test_eta.py NOAA-GFDL/pace#92

Merged

7 tasks

fmalatino requested review from FlorianDeconinck, bensonr, mlee03, oelbert and xyuan August 22, 2024 14:41

Merge branch 'develop' into feature/no_eta

104076f

fmalatino marked this pull request as ready for review August 22, 2024 14:42

FlorianDeconinck requested changes Aug 26, 2024

View reviewed changes

Comment thread ndsl/grid/eta.py Outdated

fmalatino added 2 commits August 27, 2024 14:24

Moved test_eta.py from pace/tests/main/grid into NDSL/tests/grid, add…

ba28f33

…ed eta_file generator script for testing

Merge branch 'feature/no_eta' of github.com:fmalatino/NDSL into featu…

20ed73d

…re/no_eta

fmalatino requested a review from FlorianDeconinck August 27, 2024 18:27

fmalatino added 4 commits August 27, 2024 14:33

Amending unit_tests.yaml to reflect addition of tests/grid/test_eta.py

9cafe2e

Amending file path for test_eta.py

9cb8f14

Another attempt to get the workflow to generate the test eta_files

a0c4992

Linting

10942b2

fmalatino changed the title ~~Ability to generate surface level metric terms (no eta file specified)~~ Ability to generate planar metric terms and addition of test_eta.py unit test Aug 27, 2024

oelbert requested changes Aug 29, 2024

View reviewed changes

fmalatino and others added 3 commits August 29, 2024 15:49

Use of f-string in ValueError statement of _load_ak_bk_from_file

712a385

Co-authored-by: Oliver Elbert <oliver.elbert36@gmail.com>

Amending pytest.fail statement in test_eta.py

37872f0

Co-authored-by: Oliver Elbert <oliver.elbert36@gmail.com>

Amended generation.py to define hybrid pressure variables appropriately

c76a486

oelbert reviewed Sep 4, 2024

View reviewed changes

fmalatino and others added 2 commits September 4, 2024 21:23

Amended test_eta.py docstrings to reflect updated testing methods and…

1082b62

… eta_file variable in generation.py to use an empty string in conditional statement for hybrid pressure generation

Merge branch 'develop' into feature/no_eta

8a236a7

fmalatino requested a review from oelbert September 5, 2024 13:48

fmalatino and others added 3 commits September 6, 2024 10:21

Merge branch 'develop' into feature/no_eta

71bcb56

Linting

3649691

Merge branch 'develop' into feature/no_eta

1102b75

This comment was marked as outdated.

Sign in to view

FlorianDeconinck requested changes Sep 12, 2024

View reviewed changes

Comment thread ndsl/grid/generation.py Outdated

Comment thread ndsl/grid/generation.py Outdated

fmalatino added 2 commits September 12, 2024 13:10

Amended generation.py to set default eta_file to None

1318b56

Merged most recent changes of NDSL as of 12 Sept 2024

49ebd43

fmalatino requested a review from FlorianDeconinck September 12, 2024 17:51

FlorianDeconinck requested changes Sep 12, 2024

View reviewed changes

Comment thread ndsl/grid/generation.py Outdated

Changed type hinting of eta_file in generation.py to reflect optional…

a1010dd

… aspect

fmalatino requested a review from FlorianDeconinck September 12, 2024 18:18

FlorianDeconinck approved these changes Sep 12, 2024

View reviewed changes

Merge branch 'develop' into feature/no_eta

069f056

oelbert approved these changes Sep 16, 2024

View reviewed changes

fmalatino merged commit 4a4c0c8 into NOAA-GFDL:develop Sep 16, 2024

Conversation

fmalatino commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmalatino commented Aug 22, 2024

Uh oh!

Uh oh!

FlorianDeconinck commented Aug 26, 2024

Uh oh!

oelbert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

FlorianDeconinck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

FlorianDeconinck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FlorianDeconinck left a comment

Choose a reason for hiding this comment

Uh oh!

oelbert left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fmalatino commented Aug 20, 2024 •

edited

Loading