Repository navigation
Standalone CLI tool to convert IBTrACS NetCDF files into TCTrack format - #276
sharan-monikantan wants to merge 6 commits into
Conversation
* Add abstract base class Converter * Add concrete class IBTrACSConverter for IBTrACS data * Add unit tests for IBTrACSConverter
sjavis
left a comment
sjavis
left a comment
There was a problem hiding this comment.
Thanks for your contribution it works well! I noticed some issues with the database building when testing it but I will fix those separately as they aren't really part of this issue.
I've given some suggestions for some changes before merging.
| "--tracker", | ||
| "-t", | ||
| required=True, | ||
| help="The tracker/source for the input file. Ex. IBTrACS.", |
There was a problem hiding this comment.
I think it would be better if it used 'source' instead of 'tracker'. But leave the 'tctracker_tracker' attribute unchanged so it is consistent with the TCTrack output.
|
|
||
| ```bash | ||
| tctrack-convert [-h] --tracker TRACKER [--start-date START_DATE] [--end-date END_DATE] --output-path OUTPUT_PATH INPUT_PATH | ||
| ``` |
There was a problem hiding this comment.
Add a list of all the trackers (sources). Even though this is currently just IBTrACS.
|
|
||
| Converter_ = None | ||
| for cls in Converter.__subclasses__(): | ||
| if cls.tctrack_tracker == args.tracker: |
There was a problem hiding this comment.
I would make this check case-insensitive. E.g:
| if cls.tctrack_tracker == args.tracker: | |
| if cls.tctrack_tracker.lower() == args.tracker.lower(): |
| self._convert_units_for_data_variable("wind_speed", 0.514, "m s-1") | ||
| self._convert_units_for_data_variable("air_pressure_at_sea_level", 100, "Pa") |
There was a problem hiding this comment.
Add these conversion factors as constants at the top of the file so it is more clear what they are doing. Something like:
KNOTS_TO_MPS = 0.514444
MB_TO_PA = 100
| ) | ||
| self.ds = self.ds.where( | ||
| (self.ds["time"] >= np.datetime64(f"{self.start_date}T00:00:00")) | ||
| & (self.ds["time"] <= np.datetime64(f"{self.end_date}T23:59:59")), |
There was a problem hiding this comment.
| & (self.ds["time"] <= np.datetime64(f"{self.end_date}T23:59:59")), | |
| & (self.ds["time"] < np.datetime64(f"{self.end_date}T00:00:00")), |
If we make the end date exclusive it is more consistent with how it is used elsewhere in the package.
| logging.basicConfig(level=logging.INFO, format="%(levelname)s: %(message)s", force=True) | ||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Move the logging.basicConfig inside the main function in tctrack_convert.py so it isn't initialised when this module is imported and just use logging instead of logger for simplicity.
| parser.add_argument( | ||
| "--output-path", "-o", required=True, help="Path to the output NetCDF file." | ||
| ) |
There was a problem hiding this comment.
Change this to --output so it is consistent with build_db.
| class TestGetArgumentParser: | ||
| """Tests for get_argument_parser function.""" | ||
|
|
||
| def test_parser_description(self): | ||
| """Test the description provided for the parser.""" | ||
| parser = get_argument_parser() | ||
|
|
||
| assert parser.description == "CLI tool to convert datasets into TCTrack format." | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "is_required,expected_args", | ||
| [ | ||
| (True, ["tracker", "output_path", "input_path"]), | ||
| (False, ["help", "start_date", "end_date"]), | ||
| ], | ||
| ) | ||
| def test_arguments(self, is_required, expected_args): | ||
| """Test the required and optional arguments.""" | ||
| parser = get_argument_parser() | ||
|
|
||
| actual_args = [] | ||
| for action in parser._actions: | ||
| if action.required == is_required: | ||
| actual_args.append(action.dest) | ||
|
|
||
| assert actual_args == expected_args | ||
|
|
||
| def test_argument_help_text(self): | ||
| """Test the help text for each argument.""" | ||
| expected_help_text = { | ||
| "help": "show this help message and exit", | ||
| "tracker": "The tracker/source for the input file. Ex. IBTrACS.", | ||
| "start_date": "The date (in YYYY-MM-DD format) from which the data " | ||
| "needs to be filtered from the input file.", | ||
| "end_date": "The date (in YYYY-MM-DD format) to which the data " | ||
| "needs to be filtered from the input file.", | ||
| "output_path": "Path to the output NetCDF file.", | ||
| "input_path": "Path to the input NetCDF file.", | ||
| } | ||
| parser = get_argument_parser() | ||
|
|
||
| actual_help_text = {} | ||
| for action in parser._actions: | ||
| actual_help_text[action.dest] = action.help | ||
|
|
||
| assert actual_help_text == expected_help_text |
There was a problem hiding this comment.
I don't think these are that helpful and we can drop them.
|
|
||
| assert ibtracs_converter.output_path == mock_output_path | ||
| assert ibtracs_converter.start_date == mock_start_date | ||
| assert ibtracs_converter.end_date == mock_end_date |
There was a problem hiding this comment.
This test isn't really doing anything of value. I would drop it.
This branch has not been deployed
2 participants
Resolves #215