Cut new version #30

jomey · 2021-04-23T15:39:35Z

I would like to cut a new version for the library as I:

Changed the API on to load HRRR data
Removed the forecast_flag option when loading data on get_saved_data
Renamed the output_dir parameter to input_dir on get_saved_data
Removed HRRR class.
Removed the utility method for get HRRR file names.
Cleaned up some tests.

Can we make this 0.7?
I would need guidance on how you usually cut a new release.

Then I can follow up with SMRF to either pin the version to below the suggested new one or change the one call we do from SMRF.

Update 20210424

Got carried a way a bit and changed a few more things.

FileLoader now takes the file_dir and 'file_type as initialize arguments
Renamed parameter the config_file to config
Turned down the log level in tests.
See more explanations in the commit message for each change.

This option was dropped in SMRF with PR#205

This deprecates the HRRR class, which was broken up into the data.hrrr structure in a previous commit. The class was kept around for backwards compatibility and this change will also include a version bump.

The file contained had dependencies that were not met in this repository and usage with this library has changes since.

The method has moved to the FileHandler.

scotthavens · 2021-04-23T18:57:41Z

Right now, this requires a manual push to pypi. I want to move this to Github actions and add the deployment piece to it. Then creating a release would publish to pypi. I can get to that next week, WFR isn't too complicated for testing and deployment.

scotthavens · 2021-04-23T19:02:56Z

tests/RME/RME_test_case.py

@@ -10,8 +10,7 @@ class RMETestCase(unittest.TestCase):
    """
    Base class for all unit tests using the RME data
    """
-    test_dir = Path(__file__).parent
-    basin_dir = test_dir.joinpath('RME')
+    basin_dir = Path(__file__).parent


Can you explain moving this file? It seems to go against how SMRF/AWSM are setup to test with basins. Just trying to think of how the format fits with the other tests.

I moved this in here because I see this as the next improvement over how SMRF/AWSM does it and would also like to see this change propagate to there. If you look at the class, it basically describes the structure of the basin dir test data and is all you need in order to use the data in tests.

It also clearly separates test data from test logic, moving it in there.

Last argument I see is that the test_dir property was removed and this was never used anywhere else, showing that moving the class into the folder is the better place to keep it.

Does seem to be cleaner if we move that route. Then in SMRF, the RME/Lakes would be under their presepective directories.

The one major downside I see is discoverability for that class. You pretty much have to know that it's there, as in the SMRF case, it'll be buried a few directories down.

I can see this downside addressed with documentation. Currently we have a need for that as a whole, on how they are set up, what parts tests what, or when it is appropriate to updated the gold files.

Yes to documentation. But it's way easier to just code.

weather_forecast_retrieval/data/hrrr/file_loader.py

Rename parameter in method to `file_dir` from `output_dir` and add as class property. The current implementation utilized the 'output_dir' property from the base class, which is parsing the `output_dir` value from a config file. However, calling this method should not overwrite this property.

The start and end date are already initialized in the ConfigFile base class.

There were two separate places where the dataframe was read and data manipulated. This combines them into one method.

jomey · 2021-04-23T20:52:28Z

weather_forecast_retrieval/data/hrrr/file_loader.py

-            if key == 'cloud_factor':
-                dataframe['cloud_factor'] = 1 - dataframe['cloud_factor'] / 100
-
-        return metadata, dataframe


Moved this into the convert_to_dataframes method. Seems a better place and consolidates any dataframe logic.

jomey · 2021-04-23T20:53:04Z

weather_forecast_retrieval/data/hrrr/file_loader.py

@@ -26,10 +26,8 @@ def __init__(self, config_file=None, external_logger=None):
            __name__, config_file=config_file, external_logger=external_logger
        )

-        self.start_date = None
-        self.end_date = None


Unnecessary double initialization. Already happening in the parent base class.

jomey · 2021-04-23T20:54:15Z

tests/data/hrrr/test_http_retrieval.py

@@ -3,7 +3,7 @@

 import pandas as pd

-from tests.RME_test_case import RMETestCase
+from tests.RME import RMETestCase


I see this as a readability improvement and another argument for moving the class into the folder.

Move the basic parameters for file type and file directory to the initializer. This separates the type of data from the `get_saved_data` method, where filters for the data are given with parameters.

Rename the `config_file` parameter and rename to config to support either a .ini style file or a dictionary.

Consolidate the log messages by using the same logger.

There is no need to record an initialization entry in the log when using an external logger. This also combines the logic for creating a logger and then recording the above mentioned entry.

No version of less than 3.5 is supported by this library anymore.

scotthavens · 2021-04-26T14:46:47Z

tests/RME/RME_test_case.py

@@ -10,8 +10,7 @@ class RMETestCase(unittest.TestCase):
    """
    Base class for all unit tests using the RME data
    """
-    test_dir = Path(__file__).parent
-    basin_dir = test_dir.joinpath('RME')
+    basin_dir = Path(__file__).parent


Does seem to be cleaner if we move that route. Then in SMRF, the RME/Lakes would be under their presepective directories.

The one major downside I see is discoverability for that class. You pretty much have to know that it's there, as in the SMRF case, it'll be buried a few directories down.

scotthavens · 2021-04-26T14:53:55Z

I will be working on the new release over the next couple of days. I'll also be moving to setuptools_scm to keep track of the versions.

scotthavens · 2021-04-27T15:58:48Z

v0.7.0 is up!

jomey added 5 commits April 22, 2021 14:40

HRRR - FileLoader - Remove forecast_flag parameter.

ff0cffe

This option was dropped in SMRF with PR#205

HRRR - Deprecate the HRRR class in favor of data.hrrr.

59d3d0e

This deprecates the HRRR class, which was broken up into the data.hrrr structure in a previous commit. The class was kept around for backwards compatibility and this change will also include a version bump.

Remove examples folder.

3748291

The file contained had dependencies that were not met in this repository and usage with this library has changes since.

Tests - Remove OpenDAP test - Was marked to skip.

7eef144

Tests - Move RME test base class to corresponding RME folder.

e77b0c9

jomey requested a review from scotthavens April 23, 2021 15:39

HRRR - Deprecate utils method to get a file name.

7e3826e

The method has moved to the FileHandler.

jomey force-pushed the ERW_HRRR branch from ad825c3 to 7e3826e Compare April 23, 2021 15:44

scotthavens reviewed Apr 23, 2021

View reviewed changes

jomey force-pushed the ERW_HRRR branch from 14031ef to 30fff0c Compare April 23, 2021 20:32

jomey added 3 commits April 23, 2021 14:36

HRRR - FileLoader - Remove duplicate property initialization.

cb105e6

The start and end date are already initialized in the ConfigFile base class.

HRRR - FileLoader - Consolidate logic to convert to dataframe.

1e1b175

There were two separate places where the dataframe was read and data manipulated. This combines them into one method.

HRRR - FileLoader - Update method documentation.

627415d

jomey commented Apr 23, 2021

View reviewed changes

jomey requested a review from scotthavens April 23, 2021 20:54

jomey added 6 commits April 24, 2021 09:20

HRRR - FileLoader - Further refine the parameters.

87c6a4d

Move the basic parameters for file type and file directory to the initializer. This separates the type of data from the `get_saved_data` method, where filters for the data are given with parameters.

HRRR - ConfigFile - Improve checking for config entries presence.

fa90008

HRRR - ConfigFile - Refactor config_file parameter.

33cab7c

Rename the `config_file` parameter and rename to config to support either a .ini style file or a dictionary.

HRRR - FileLoader - Pass the logger to the file loader.

ae0ad6d

Consolidate the log messages by using the same logger.

Tests - turn down the noise level a bit and seg lot to ERROR.

22bb95e

Address lint errors.

56e8e4e

jomey force-pushed the ERW_HRRR branch from dce09c4 to 56e8e4e Compare April 24, 2021 16:53

jomey added 2 commits April 25, 2021 10:42

HRRR - ConfigFile - improve logger handling.

23d6344

There is no need to record an initialization entry in the log when using an external logger. This also combines the logic for creating a logger and then recording the above mentioned entry.

Utils - read_config - Drop check for Python version.

03377a0

No version of less than 3.5 is supported by this library anymore.

scotthavens approved these changes Apr 26, 2021

View reviewed changes

scotthavens merged commit 949df46 into USDA-ARS-NWRC:main Apr 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cut new version #30

Cut new version #30

jomey commented Apr 23, 2021 •

edited

scotthavens commented Apr 23, 2021

scotthavens Apr 23, 2021

jomey Apr 23, 2021

scotthavens Apr 26, 2021

jomey Apr 26, 2021

scotthavens Apr 26, 2021

jomey Apr 23, 2021

jomey Apr 23, 2021

jomey Apr 23, 2021

scotthavens Apr 26, 2021

scotthavens commented Apr 26, 2021

scotthavens commented Apr 27, 2021

Cut new version #30

Cut new version #30

Conversation

jomey commented Apr 23, 2021 • edited

Update 20210424

scotthavens commented Apr 23, 2021

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

scotthavens commented Apr 26, 2021

scotthavens commented Apr 27, 2021

jomey commented Apr 23, 2021 •

edited