AdfData class for streamlined I/O in plotting scripts #269

brianpm · 2023-12-01T20:55:52Z

This PR introduces a new class called ADFData in lib/adf_dataset.py. This class is instantiated with an ADFDiag instance. It is intended to hold most of the boilerplate code that is in the plotting scripts to set paths and load data. It also abstracts the treatment of "obs" and "baseline" into something that looks more unified (at least to the plotting scripts). The upshot is that using this class can reduce the amount of code in plotting scripts. As examples, I've added zonal_mean_B.py and global_latlon_map_B.py. These are refactors of the current versions that now use ADFData, and both are quite a bit shorter in the B versions. In limited testing, I think they are producing the same output.

NOTE I see that create_climo_files.py also appears to be changed here. I don't know why. That should not be part of this PR... I did a sync with ADF so I thought I was completely up to date. I think the changes in that file were developed to help @justin-richling with an issue with the climatology years. The code looks right, but I wasn't intending it to be part of this PR. (It can be if we double check it.)

…maps scripts

nusbaume

Fantastic cleanup! However, I did have some change requests, specifically about moving the new AdfData object to inside the AdfDiag class, and simply updating the current plotting scripts instead of adding new *_B.py scripts.

Otherwise the rest of my change requests should be pretty minor, although feel free to push back if something ends up being more difficult than expected. Thanks!

lib/adf_dataset.py

scripts/plotting/global_latlon_map_B.py

scripts/plotting/zonal_mean_B.py

…data for derived time series.

brianpm · 2024-06-12T23:07:03Z

@nusbaume -- Okay. I think I've got an updated version that is working.

I didn't rename the scripts because I'd like @justin-richling to confirm that the plots are actually working correctly.

In my limited testing using recent development runs, I had to also update adf_info.py because of the history file changes. So now it checks for time_bnds OR time_bounds and uses the hist_str to look for time series files. Feel free to exclude these changes is they are being handled by a different PR.

I also ran into the issue where doing the derived time series was hanging, so I added .compute() in adf_diag.py. If there's a more robust fix for that, this change can also be ignored.

nusbaume

This PR looks good to me now! I have a few, hopefully trivial, change requests, but I trust that hey can be resolved without a re-review from me. I also left the one comment un-resolved about replacing the lat-lon and zonal mean plots, as I figured we won't want to do this until @justin-richling has successfully tested the new routines.

Finally, my hope is that if we can get PR #310 merged in soon, and then merge this PR to the head of the ADF, that it will fix the current test failures. Of course if that turns out not to be the case then I am happy to take on getting those tests fixed before this PR is officially brought in.

Thanks!

nusbaume · 2024-06-16T20:06:54Z

lib/adf_dataset.py

@@ -131,7 +129,7 @@ def load_climo_file(self, case, variablename):
        """Return Dataset for climo of variablename"""
        fils = self.get_climo_file(case, variablename)
        if not fils:
-            print(f"ERROR: Did not find climo file for variable: {variablename}. Will try to skip.")
+            warnings.warning(f"ERROR: Did not find climo file for variable: {variablename}. Will try to skip.")


I think (?) this should be warn not warning:

Suggested change

warnings.warning(f"ERROR: Did not find climo file for variable: {variablename}. Will try to skip.")

warnings.warn(f"ERROR: Did not find climo file for variable: {variablename}. Will try to skip.")

Yup. Fixed it.

nusbaume · 2024-06-16T21:32:55Z

lib/adf_info.py

+            except:
+                print(" -----------    ERROR   ------------")
+                print(ts_files)


I think this is OK for now, but do you remember what the specific error was that prompted this try/except? In general we want to try and check for a specific type of exception, otherwise it could catch potential errors that we actually want to be exposed (for example if the computer itself has a system/hardware error).

Sorry, this is a remnant of me trying to do testing. I'm taking it out. I was hitting a problem where multiple time series files had been generated for case (and put in the same directory). It resulted in having multiple files identified in the search that couldn't be concatenated because they had overlapping times. Maybe that needs to be revisited sometime, but I don't think this check needs to be included here.

nusbaume · 2024-06-16T21:50:12Z

scripts/plotting/global_latlon_map_B.py

@@ -83,7 +82,7 @@ def global_latlon_map_B(adfobj):
    #
    # Use ADF api to get all necessary information
    #
-    data = AdfData(adfobj)
+    # data = AdfData(adfobj) NO LONGER NEEDED


Assuming this script works as expected I would just delete this line.

nusbaume · 2024-06-16T21:51:38Z

scripts/plotting/zonal_mean_B.py

@@ -47,7 +46,7 @@ def zonal_mean_B(adfobj):

    # Start by instantiating the AdfData object
    # and Extract needed quantities from ADF object:
-    data = AdfData(adfobj)
+    # data = AdfData(adfobj)


Assuming this script works as expected I would just delete this line.

justin-richling · 2024-06-21T17:05:30Z

@brianpm Looks like the plots for Lat/Lon and Zonal are the same between the old way and this way. Thanks for the clean up of the code!

justin-richling

Looks good to me, thanks for the work on this!

brianpm · 2024-06-21T21:27:19Z

Okay, I think I've got everything in. It says there's conflicts with adf_info.py, but I think we want this version (@justin-richling took a look on my previous commit and noted that we would want a couple of the changes that I had accidentally removed that affect hist_str and time_bounds).

brianpm added 30 commits October 26, 2023 21:14

sketching out a path for AdfData; adf_dataset.py and zonal_Mean_B.py

b28b6ac

working on making it run

796a8c0

fix getting reference case path

18706b3

allow reference case to have a label

efdfba5

adfdata applied to global maps script

6456fdc

adfdata applied to global maps script -- rename function

aababb2

adfdata applied to global maps script -- debugging

988011b

adfdata applied to global maps script -- debugging

91bce2a

adfdata applied to global maps script -- debugging

bf02a1f

debugging

71578ef

debugging

06d3ea1

debugging

11f1477

debugging

c7286b7

debugging

97e1cbd

debugging -- fix load_da; actually return the DataArray

9068ec9

debugging

ceecec9

debugging -- fix load_reference_da; actually return the DataArray

b48d01a

debugging

dd015ef

debugging

84150a4

debugging

42614fb

debugging

b7765ab

debugging

857a441

debugging

577c9e7

debugging

87aebf2

debugging

c751d02

completed updates for AdfData and implement in zonal mean and global …

f9f296d

…maps scripts

remove extra debugging print statements

9ec949b

refactor plot_file_op for easier logic

3adbfe0

seasonal averaging moved to inside season loop

51394a6

seasonal averaging moved to inside season loop

cf2128c

nusbaume self-requested a review May 10, 2024 19:54

nusbaume added code clean-up Made code simpler and/or easier to read. plotting Related to plot generation labels May 10, 2024

nusbaume requested changes May 14, 2024

View reviewed changes

brianpm and others added 5 commits June 12, 2024 14:43

starting to address PR comments. First round done on adf_dataset.py

5d46906

merged my conflicted adf_dataset.py

ce50839

addressing PR comments. Instantiate AdfData from AdfDiag

142d4e5

Merge branch 'NCAR:main' into adf_case_dataclass

935b6f4

testing AdfData. Fixes for history file naming. Fix to force loading …

cae606f

…data for derived time series.

removed commented lines

0183799

nusbaume approved these changes Jun 16, 2024

View reviewed changes

nusbaume requested a review from justin-richling June 16, 2024 22:03

addressing Jesse's comments on PR

26a059c

justin-richling approved these changes Jun 21, 2024

View reviewed changes

brianpm added 4 commits June 21, 2024 13:28

Replace and with versions using new class

7d6cdc9

try to merge amwg_table from ADF/main

df14b1b

resolve upstream conflicts

b966637

add back useful changes to adf_info

39da27e

justin-richling and others added 7 commits June 21, 2024 15:29

Merge branch 'main' into adf_case_dataclass

2d5a7dc

Merge branch 'main' into adf_case_dataclass

de213fc

bug fixes.

231aaaa

correct arguments for load_reference_regrid_da

6e20dae

Merge branch 'main' into adf_case_dataclass

496d5c7

trying to fix linting errors

e36585f

added load method for timeseries files

febddf5

justin-richling merged commit f8a8959 into NCAR:main Jul 11, 2024
7 checks passed

justin-richling mentioned this pull request Jul 16, 2024

Dataclass fixes #317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AdfData class for streamlined I/O in plotting scripts #269

AdfData class for streamlined I/O in plotting scripts #269

brianpm commented Dec 1, 2023

nusbaume left a comment

brianpm commented Jun 12, 2024

nusbaume left a comment

nusbaume Jun 16, 2024

brianpm Jun 18, 2024

nusbaume Jun 16, 2024

brianpm Jun 18, 2024

nusbaume Jun 16, 2024

brianpm Jun 18, 2024

nusbaume Jun 16, 2024

brianpm Jun 18, 2024

justin-richling commented Jun 21, 2024

justin-richling left a comment

brianpm commented Jun 21, 2024

	warnings.warning(f"ERROR: Did not find climo file for variable: {variablename}. Will try to skip.")
	warnings.warn(f"ERROR: Did not find climo file for variable: {variablename}. Will try to skip.")

AdfData class for streamlined I/O in plotting scripts #269

AdfData class for streamlined I/O in plotting scripts #269

Conversation

brianpm commented Dec 1, 2023

nusbaume left a comment

Choose a reason for hiding this comment

brianpm commented Jun 12, 2024

nusbaume left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justin-richling commented Jun 21, 2024

justin-richling left a comment

Choose a reason for hiding this comment

brianpm commented Jun 21, 2024