Pressure masking tool fixes, tests, and docs #406

ceblanton · 2025-04-22T14:40:08Z

The metadata reading and writing needs to be finalized now that the history files are being written with the intended variable attributes.

Describe your changes

Issue ticket number and link (if applicable)

Checklist before requesting a review

I ran my code
I tried to make my code readable
I tried to comment my code
I wrote a new test, if applicable
I wrote new instructions/documentation, if applicable
I ran pytest and inspected it's output
I ran pylint and attempted to implement some of it's feedback
No print statements; all user-facing info uses logging module

The metadata reading and writing needs to be finalized now that the history files are being written with the variable attribute updates.

ilaflott

some clarity could help in current spots

fre/app/mask_atmos_plevel.py

and added comment to explain the if statement logic.

laurenchilutti

Looks good! I added a few comments.

fre/app/freapp.py

fre/app/mask_atmos_level.md

fre/app/mask_atmos_plevel.py

fre/app/tests/test_mask_atmos_plevel.py

fre/app/freapp.py

- moved setup to its own fixture - added reference test with known good output - modified input file's metadata to turn on the masking - added type to docstring

codecov · 2025-07-14T20:28:06Z

Codecov Report

Attention: Patch coverage is 93.44262% with 4 lines in your changes missing coverage. Please review.

Project coverage is 66.88%. Comparing base (e4ee6cf) to head (7597ab0).

Files with missing lines	Patch %	Lines
fre/app/mask_atmos_plevel.py	82.35%	3 Missing ⚠️
fre/app/freapp.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #406      +/-   ##
==========================================
+ Coverage   65.48%   66.88%   +1.39%     
==========================================
  Files          63       64       +1     
  Lines        4532     4560      +28     
==========================================
+ Hits         2968     3050      +82     
+ Misses       1564     1510      -54

Flag	Coverage Δ
unittests	`66.88% <93.44%> (+1.39%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ilaflott

This is close to being merge-able.

I think we need this code in a folder like the other fre app commands, otherwise the testing file pattern is broken with mask-atmos-plevel being the exception.

Does the tool require nco? Do requirements in meta.yaml, environment.yml, or setup.py need to be changed because of this?

ilaflott · 2025-07-15T16:51:53Z

fre/app/README.md

fre/app/README.md should be a README for fre app generally, and not for fre app mask-atmos-plevel/

Thanks Ian on all counts.

moved the mask-atmos-plevel code to its own subdirectory which is more consistent with others, agreed.

Great catch on the prereqs, which are xarray and netCDF4. xarray is already there but netcdf is not. I'll add it

README.md already ok

I think you should move this readme to fre/app/mask_atmos_plevel directory and keep it subtool specific, similar to what Dana did in fre/app/remap_pp_components/README.md and https://github.com/NOAA-GFDL/fre-cli/blob/main/fre/app/regrid_xy/README.md

…consistent with others

…os-plevel

mlee03

First round of reviews hehehe

mlee03 · 2025-07-23T13:57:04Z

fre/app/README.md

+# mask-atmos-plevel: Mask pressure-level diagnostic output below land surface
+Quickstart:
+
+First, obtain an input file that has a pressure coordinate variable and


I'm not a fan of using first and second person in official documents.. :)
Also may I suggest, input file containing pressure coordinate variable and data variable that is to be masked. The mask value will be set to 1 at points where the pressure is greater than the surface pressure. I'm not sure what "which is specified in either the first file or a second" is referring to..

mlee03 · 2025-07-23T13:57:43Z

fre/app/README.md

@@ -0,0 +1,27 @@
+# fre app - Loose standalone tools
+
+# mask-atmos-plevel: Mask pressure-level diagnostic output below land surface


I'ts not clear what mask-atmos-plevel does. Does it compute? Evaluate? Determine?

Thanks, I'll improve the summary line. It applies a boolean mask for pressure-level input data where the condition is surface pressure. For pressures above (lower) than the condition, the data is set to the missing value. For pressures equal or less than surface pressure, use the input data.

mlee03 · 2025-07-24T14:15:13Z

fre/app/README.md

+atmos_cmip.200501-200512.ps.nc  atmos_cmip.200501-200512.ua_unmsk.nc
+```
+
+Then, add a variable attribute "pressure_mask" to the data variable you wish to mask.


Maybe instead of sequences of "ten", each step can be enumerated such as (1) something (2) something ...

mlee03 · 2025-07-24T14:19:29Z

fre/app/mask_atmos_plevel/mask_atmos_plevel.py

+    :raises FileNotFound: Input file does not exist
+    :rtype: None
+
+    .. note:: Input variables must have an attribute `pressure_mask` set to `False`. Output variables have the attribute set to `True`.


Input variables must have the attribute pressure_mask that is set to False. The resulting output variable will have this attribute set to True.

mlee03 · 2025-07-24T14:19:41Z

fre/app/mask_atmos_plevel/mask_atmos_plevel.py

+fre_logger = logging.getLogger(__name__)
+
+def mask_atmos_plevel_subtool(infile: str, psfile: str, outfile: str) -> None:
+    """Mask pressure-level diagnostic output below land surface


new line after """

Thank you! I didn't realize the distinction and have been doing it wrong until now.

mlee03 · 2025-07-24T14:20:50Z

fre/app/mask_atmos_plevel/mask_atmos_plevel.py

+
+
+def pressure_coordinate(ds: xr.Dataset, varname: str) -> xr.DataArray:
+    """Check if dataArray has pressure coordinate fitting requirements


should also be Checks instead of check?

It's not sure what "return it" means, perhaps replace "it" with what it is referring to

mlee03 · 2025-07-24T14:21:56Z

fre/app/mask_atmos_plevel/mask_atmos_plevel.py

+
+
+def write_dataset(ds: xr.Dataset, template: xr.Dataset, outfile: str) -> None:
+    """Prepare the dataset and write output NetCDF file


"write output" can be simplified to "outputs"

mlee03 · 2025-07-24T14:22:34Z

fre/app/tests/test_mask_atmos_plevel.py

+
+@pytest.fixture()
+def create_input_files(tmp_path):
+    """Create input data file atmos_cmip.ua_unmsk.nc and ps file atmos_cmip.ps.nc


also instead of "returning the temporary directory", "returning the name of the temporary directory"

mlee03 · 2025-07-24T14:23:02Z

fre/app/tests/test_mask_atmos_plevel.py

+
+
+def test_mask_atmos_plevel(create_input_files):
+    """Do the pressure masking on the test input file,


mlee03 · 2025-07-24T14:23:18Z

fre/app/tests/test_mask_atmos_plevel.py

+
+def test_mask_atmos_plevel(create_input_files):
+    """Do the pressure masking on the test input file,
+    and then compare to the known reference output file.


not sure what it is comparing to

mlee03 · 2025-07-24T14:25:42Z

Also, this tool is using both Xarray and Netcdf, perhaps there should be an effort to swap all Netcdf usage to Xarray.
The test input files could also be generated on the fly too...

laurenchilutti · 2025-07-28T13:56:38Z

fre/app/README.md

I think you should move this readme to fre/app/mask_atmos_plevel directory and keep it subtool specific, similar to what Dana did in fre/app/remap_pp_components/README.md and https://github.com/NOAA-GFDL/fre-cli/blob/main/fre/app/regrid_xy/README.md

ilaflott · 2025-08-15T15:19:30Z

@ceblanton merge conflict fixed.

Adjustments for pressure masking tool, beginnings of.

0c38d25

The metadata reading and writing needs to be finalized now that the history files are being written with the variable attribute updates.

ilaflott self-requested a review May 6, 2025 16:34

ilaflott reviewed May 6, 2025

View reviewed changes

fre/app/mask_atmos_plevel.py Outdated Show resolved Hide resolved

fre/app/mask_atmos_plevel.py Outdated Show resolved Hide resolved

Chris Blanton and others added 9 commits July 1, 2025 18:29

Started adding docstrings for fre app mask-atmos-plevel

cfb3fcf

Set logging level through the fre -v calls (-vv for debug)

fbb8d11

Improved debug messages for why the input file was not processed,

e0aefd3

and added comment to explain the if statement logic.

Good attempt for docstrings and type hints for fre app mask-atmos-plevel

ae06c2c

Fixed logic

e1401c1

more docstrings

c4f0555

add quickstart for fre app mask-atmos-plevel

ddbe925

Merge remote-tracking branch 'origin/main' into mask-atmos-plevel

22dd045

Started pytest for mask-atmos-plevel

b9df0d3

ceblanton changed the title ~~Adjustments for pressure masking tool~~ Pressure masking tool fixes, tests, and docs Jul 2, 2025

This was referenced Jul 2, 2025

docs: fre app mask-atmos-plevel README ? #482

Open

docs: mask_atmos_plevel.py #486

Open

laurenchilutti requested changes Jul 2, 2025

View reviewed changes

laurenchilutti reviewed Jul 2, 2025

View reviewed changes

fre/app/freapp.py Outdated Show resolved Hide resolved

Chris Blanton added 3 commits July 8, 2025 18:33

Moved mask-atmos-plevel quickstart to README.md

e981f40

Removed underscores from docstrings

61fa1df

Finished mask atmos plevel test

0a15c33

- moved setup to its own fixture - added reference test with known good output - modified input file's metadata to turn on the masking - added type to docstring

ceblanton marked this pull request as ready for review July 8, 2025 23:32

ceblanton requested review from singhd789 and laurenchilutti July 8, 2025 23:32

ceblanton mentioned this pull request Jul 8, 2025

Use the fre-cli version of the atmos masking script NOAA-GFDL/fre-workflows#64

Open

Merge branch 'main' into mask-atmos-plevel

7597ab0

ilaflott requested changes Jul 15, 2025

View reviewed changes

Chris Blanton added 3 commits July 22, 2025 18:51

Move mask-atmos-plevel code to its own subdirectory in fre/app to be …

88f53e1

…consistent with others

Merge remote-tracking branch 'origin/mask-atmos-plevel' into mask-atm…

5c2ee1b

…os-plevel

Added netcdf4 prerequisite needed for mask-atmos-plevel

46cbe23

singhd789 mentioned this pull request Jun 25, 2025

WF Testing: mask-atmos-plevel NOAA-GFDL/fre-workflows#102

Open

mlee03 suggested changes Jul 24, 2025

View reviewed changes

laurenchilutti requested changes Jul 28, 2025

View reviewed changes

laurenchilutti mentioned this pull request Jul 28, 2025

docs: fre/app/*/README's ? #538

Closed

Merge branch 'main' into mask-atmos-plevel

b56bc53

ilaflott assigned ceblanton Aug 12, 2025

ilaflott added documentation Improvements or additions to documentation new functioning New feature or request clean up labels Aug 15, 2025

Merge branch 'main' into mask-atmos-plevel

4164533

		@@ -0,0 +1,27 @@
		# fre app - Loose standalone tools

		# mask-atmos-plevel: Mask pressure-level diagnostic output below land surface



		def pressure_coordinate(ds: xr.Dataset, varname: str) -> xr.DataArray:
		"""Check if dataArray has pressure coordinate fitting requirements



		def write_dataset(ds: xr.Dataset, template: xr.Dataset, outfile: str) -> None:
		"""Prepare the dataset and write output NetCDF file



		def test_mask_atmos_plevel(create_input_files):
		"""Do the pressure masking on the test input file,

Pressure masking tool fixes, tests, and docs #406

Are you sure you want to change the base?

Pressure masking tool fixes, tests, and docs #406

Uh oh!

Conversation

ceblanton commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Issue ticket number and link (if applicable)

Checklist before requesting a review

Uh oh!

ilaflott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

laurenchilutti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ilaflott left a comment

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!

mlee03 left a comment

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

mlee03 commented Jul 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilaflott commented Aug 15, 2025

Uh oh!

Uh oh!

ceblanton commented Apr 22, 2025 •

edited

Loading

codecov bot commented Jul 14, 2025 •

edited

Loading