Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary try except #104

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@
/build
/documentation

*.log
*.log
.coverage
6 changes: 3 additions & 3 deletions fm2prof/CrossSection.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import numpy as np
import pandas as pd
import scipy.optimize as so
from scipy.integrate import cumtrapz
from scipy.integrate import cumulative_trapezoid
from tqdm import tqdm

from fm2prof import Functions as FE
Expand Down Expand Up @@ -374,10 +374,10 @@ def get_timeseries(name: str):

# Compute 1D volume as integral of width with respect to z times length
self._css_total_volume = np.append(
[0], cumtrapz(self._css_total_width, self._css_z) * self.length
[0], cumulative_trapezoid(self._css_total_width, self._css_z) * self.length
)
self._css_flow_volume = np.append(
[0], cumtrapz(self._css_flow_width, self._css_z) * self.length
[0], cumulative_trapezoid(self._css_flow_width, self._css_z) * self.length
)

# If sd correction is run, these attributes will be updated.
Expand Down
6 changes: 3 additions & 3 deletions fm2prof/IniFile.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ def __init__(self, file_path: Union[Path, str] = ".", logger: Logger = None):

if file_path is None:
self.set_logger_message(
f"No ini file given, using default options", "warning"
"No ini file given, using default options", "warning"
)
return
else:
Expand All @@ -113,7 +113,7 @@ def __init__(self, file_path: Union[Path, str] = ".", logger: Logger = None):
self._read_inifile(file_path)
elif file_path.is_dir():
self.set_logger_message(
f"No ini file given, using default options", "warning"
"No ini file given, using default options", "warning"
)
pass
else:
Expand All @@ -122,7 +122,7 @@ def __init__(self, file_path: Union[Path, str] = ".", logger: Logger = None):

@property
def _file_dir(self) -> Path:
if not self._file is None:
if self._file is not None:
return self._file.parent
return Path().cwd()

Expand Down
60 changes: 33 additions & 27 deletions fm2prof/MaskOutputFile.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,68 +20,74 @@
Stichting Deltares and remain full property of Stichting Deltares at all times.
All rights reserved.
"""

import os

from pathlib import Path
from typing import Union, Optional
import geojson



class MaskOutputFile:
@staticmethod
def create_mask_point(coords: geojson.coords, properties: dict):
def create_mask_point(coords: geojson.coords, properties: Optional[dict] = None) -> geojson.Feature:
"""Creates a Point based on the properties and coordinates given.

Arguments:
coords {geojson.coords} --
Coordinates tuple (x,y) for the mask point.
properties {dict} -- Dictionary of properties

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
properties {dict} -- Dictionary of properties
properties {dict} -- Dictionary of properties
properties {dict, optional} -- Dictionary of properties. Defaults to None.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contributing page of the documentation suggets to use google style docstrings when writing new code. The docstrings throughout the code are not consistent at the moment. I'm working on a PR that will address this issue.

"""
output_mask = geojson.Feature(geometry=geojson.Point(coords))
if not coords:
raise ValueError("coords cannot be empty.")
output_mask = geojson.Feature(geometry=geojson.Point(coords))

if properties:
output_mask.properties = properties
return output_mask

@staticmethod
def validate_extension(file_path: str):
if not file_path:
# Should not be evaluated
return
if not file_path.endswith(".json") and not file_path.endswith(".geojson"):
def validate_extension(file_path: Union[str, Path]) -> None:
if not isinstance(file_path, (str, Path)):
err_msg = f"file_path should be string or Path, not {type(file_path)}"
raise TypeError(err_msg)
if Path(file_path).suffix not in [".json", ".geojson"]:
raise IOError(
"Invalid file path extension, " + "should be .json or .geojson."
"Invalid file path extension, should be .json or .geojson."
)

@staticmethod
def read_mask_output_file(file_path: str):
def read_mask_output_file(file_path: Union[str, Path]) -> dict:
"""Imports a GeoJson from a given json file path.

Arguments:
file_path {str} -- Location of the json file
Copy link

@tim-vd-aardweg tim-vd-aardweg Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
file_path {str} -- Location of the json file
file_path {str} -- Location of the json file
file_path {Union[str, Path]} -- Location of the json file

"""
geojson_data = geojson.FeatureCollection(None)
if not file_path or not os.path.exists(file_path):
return geojson_data
file_path = Path(file_path)
if not file_path.exists():
err_msg = f"File path {file_path} not found."
raise FileNotFoundError(err_msg)

MaskOutputFile.validate_extension(file_path)
with open(file_path) as geojson_file:
try:
geojson_data = geojson.load(geojson_file)
except:
return geojson_data

with file_path.open("r") as geojson_file:
geojson_data = geojson.load(geojson_file)
if not isinstance(geojson_data, geojson.FeatureCollection):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check actually needed? Can geojson_data be something else than a geojson.FeatureCollection?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, geojson will load a json as a dict. If the file is a geojson it will return a geojson.FeatureCollection

raise IOError("File is empty or not a valid geojson file.")
return geojson_data

@staticmethod
def write_mask_output_file(file_path: str, mask_points: list):
def write_mask_output_file(file_path: Union[Path, str], mask_points: list) -> None:
"""Writes a .geojson file with a Feature collection containing
the mask_points list given as input.

Arguments:
file_path {str} -- file_path where to store the geojson.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
file_path {str} -- file_path where to store the geojson.
file_path {str} -- file_path where to store the geojson.
file_path {Union[Path, str]} -- file_path where to store the geojson.

mask_points {list} -- List of features to output.
"""
if file_path:
MaskOutputFile.validate_extension(file_path)
feature_collection = geojson.FeatureCollection(mask_points)
with open(file_path, "w") as f:
geojson.dump(feature_collection, f, indent=4)
if not file_path:
raise ValueError("file_path is required.")
file_path = Path(file_path)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should first check if the given file_path is not None. If it is, Path(file_path) may (or may not, depending on the python version) raise an error. So better to do something like:

        if not file_path:
            raise ValueError("file_path is required.")
        if not mask_points:
            raise ValueError("mask_points cannot be empty.")
        file_path = Path(file_path)

Copy link
Collaborator Author

@Tjalling-dejong Tjalling-dejong Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, I added the check.

if not mask_points:
raise ValueError("mask_points cannot be empty.")
MaskOutputFile.validate_extension(file_path)
feature_collection = geojson.FeatureCollection(mask_points)
with file_path.open("w") as f:
geojson.dump(feature_collection, f, indent=4)
3 changes: 0 additions & 3 deletions tests/TestUtils.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
import contextlib
import os
import shutil
import sys
from pathlib import Path
from typing import List

import pytest

Expand Down
40 changes: 11 additions & 29 deletions tests/test_Import.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
import numbers
import os
import shutil
import sys
import unittest
from pathlib import Path

import pytest
Expand Down Expand Up @@ -89,11 +84,9 @@ def test_when_given_expected_arguments_then_object_is_created(self):
css_data,
]
return_fm_model_data = None
# 2. Run test
try:
return_fm_model_data = FmModelData(arg_list)
except:
pytest.fail("No exception expected but was thrown")

# 2. Run test
return_fm_model_data = FmModelData(arg_list)

# 4. Verify final expectations
assert return_fm_model_data is not None
Expand Down Expand Up @@ -127,12 +120,9 @@ def test_when_given_data_dictionary_then_css_data_list_is_set(self):
# 2. Set expectations
expected_css_data_list = [{dummy_key: 0}, {dummy_key: 1}]

# 3. Run test
try:
return_fm_model_data = FmModelData(arg_list)
except:
pytest.fail("No exception expected but was thrown")

# 3. Run test
return_fm_model_data = FmModelData(arg_list)

# 4. Verify final expectations
assert return_fm_model_data is not None
assert return_fm_model_data.css_data_list != css_data_dict
Expand All @@ -153,11 +143,8 @@ def test_when_given_dictionary_then_returns_list(self):
expected_list = [{dummy_key: 0}, {dummy_key: 1}]

# 3. Run test
try:
return_list = FmModelData.get_ordered_css_list(test_dict)
except:
pytest.fail("No exception expected but was thrown")

return_list = FmModelData.get_ordered_css_list(test_dict)

# 4. Verify final expectations
assert return_list is not None
assert return_list == expected_list, (
Expand All @@ -170,18 +157,13 @@ def test_when_given_dictionary_then_returns_list(self):
def test_when_given_unexpected_value_then_returns_empty_list(self, test_dict):
# 1. Set up test_data
return_list = None
dummy_key = "dummyKey"
dummy_values = [0, 1]

# 2. Run test
expected_list = []

# 3. Run test
try:
return_list = FmModelData.get_ordered_css_list(test_dict)
except:
pytest.fail("No exception expected but was thrown")

# 3. Run test
return_list = FmModelData.get_ordered_css_list(test_dict)

# 4. Verify final expectations
assert return_list is not None
assert return_list == expected_list, (
Expand Down
Loading
Loading