-
Notifications
You must be signed in to change notification settings - Fork 8
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
[WIP] Add additional linting to package #10
base: main
Are you sure you want to change the base?
Conversation
@SorooshMani-NOAA I'd like your feedback specifically about how to handle the remaining errors and your thoughts about integrating tests for these in the Github Action testing workflow. |
# refine_opts.mesh_top1 = True | ||
# refine_opts.geom_feat = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove extra spaces between # and the commented code following it now that the lines are shifted
# if out_crs is not None: | ||
# utils.reproject(jig_remeshed, out_crs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, please remove extra spaces
"--contour", | ||
action="append", | ||
nargs="+", | ||
type=float, | ||
dest="contours", | ||
metavar="CONTOUR_DEFN", | ||
default=[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's too much to spread every argument to a newline. Is there any way to control linter not to do this?
from ocsmesh import Geom, Hfun, JigsawDriver, Raster | ||
from ocsmesh.features.contour import Contour | ||
from ocsmesh.geom.shapely import MultiPolygonGeom | ||
from ocsmesh.hfun.mesh import HfunMesh | ||
from ocsmesh.features.contour import Contour | ||
from ocsmesh.mesh.mesh import Mesh | ||
from ocsmesh.mesh.parsers import sms2dm | ||
from ocsmesh.utils import msh_t_to_2dm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better if we have a 3rd block of imports specific to the module within this package (ocsmesh) and then organize them separately instead of mixing with other third_party
raster_path, | ||
raster_opts, | ||
zmin, | ||
zmax, | ||
join_method, | ||
driver, | ||
chunk_size, | ||
overlap, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how I feel about removing 1 level of indentation here. It looks OK, but I don't know if it causes more confusion or not do to aligning with function body
self, | ||
geom: Geom, | ||
hfun: Hfun, | ||
initial_mesh: bool = False, | ||
crs: Union[str, CRS] = None, | ||
verbosity: int = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same issue with indentation aligning with function body
# if self.opts.hfun_hmin > 0: | ||
# output_mesh = utils.remesh_small_elements( | ||
# self.opts, geom_msh_t, output_mesh, hfun_msh_t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove extras spaces after # now that the lines are shifted
self, | ||
value, | ||
upper_bound=np.inf, | ||
lower_bound=-np.inf, | ||
value_type: str = "min", | ||
rate=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment!
self, | ||
function=lambda i: i / 2.0, | ||
upper_bound=np.inf, | ||
lower_bound=-np.inf, | ||
value_type: str = "min", | ||
rate=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just marking as same comment!
@@ -50,11 +52,10 @@ def iter_contours(self): | |||
def _get_contour_from_source(self, source): | |||
pass | |||
|
|||
# @abstractmethod | |||
# @abstractmethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra spaces after #
self, | ||
level0=None, | ||
level1=None, | ||
sources=[], | ||
max_contour_defn: Contour = None, | ||
shapefile=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation alignment thing!
self, | ||
shape: Union[None, MultiPolygon, Polygon] = None, | ||
shape_crs: CRS = CRS.from_user_input("EPSG:4326"), | ||
shapefile: Union[None, str, Path] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation alignment
self, | ||
in_list: Sequence[ | ||
Union[str, Raster, RasterGeom, MeshGeom, MultiPolygonGeom, PolygonGeom] | ||
], | ||
base_mesh: Mesh = None, | ||
zmin: float = None, | ||
zmax: float = None, | ||
nprocs: int = None, | ||
chunk_size: int = None, | ||
overlap: int = None, | ||
verbosity: int = 0, | ||
base_shape: Union[Polygon, MultiPolygon] = None, | ||
base_shape_crs: Union[str, CRS] = "EPSG:4326", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argument and body indentation alignment
self, | ||
shape: Union[MultiPolygon, Polygon] = None, | ||
level: Union[Tuple[float, float], float] = None, | ||
contour_defn: Union[FilledContour, Contour] = None, | ||
patch_defn: Patch = None, | ||
shapefile: Union[None, str, Path] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just marking the same indentation thing since I noticed it!
self, | ||
raster: Union[Raster, str, os.PathLike], | ||
zmin=None, | ||
zmax=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just marking!
@@ -48,7 +48,8 @@ def __init__( | |||
self._zmax = zmax | |||
|
|||
def get_multipolygon( # type: ignore[override] | |||
self, zmin: float = None, zmax: float = None) -> MultiPolygon: | |||
self, zmin: float = None, zmax: float = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation alignment thing again! Maybe I should ignore this?
self, | ||
multipolygon: Union[MultiPolygon, Polygon], | ||
expansion_rate: float = None, | ||
target_size: float = None, | ||
nprocs: int = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation alignment
# if self.hmin is not None: | ||
# values[np.where(values < self.hmin)] = self.hmin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra spaces after #
self, | ||
feature: Union[LineString, MultiLineString], | ||
expansion_rate: float, | ||
target_size: float = None, | ||
max_verts=200, | ||
*, # kwarg-only comes after this | ||
pool: Pool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation alignement thing again
# if self.hmin is not None: | ||
# values[np.where(values < self.hmin)] = self.hmin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra spaces
self, | ||
window: rasterio.windows.Window = None, | ||
marche: bool = False, | ||
verbosity=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking for similar comment
self, | ||
func=lambda i: i / 2.0, | ||
upper_bound=np.inf, | ||
lower_bound=-np.inf, | ||
value_type: str = "min", | ||
rate=0.01, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation alignment
self, | ||
multipolygon: Union[MultiPolygon, Polygon], | ||
expansion_rate: float = None, | ||
target_size: float = None, | ||
nprocs: int = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation alignment
self, | ||
level: Union[List[float], float], | ||
expansion_rate: float, | ||
target_size: float = None, | ||
nprocs: int = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indenatation alignment
self, | ||
in_list: Sequence[Union[str, Raster, Mesh, HfunRaster, HfunMesh]], | ||
base_mesh: Mesh = None, | ||
hmin: float = None, | ||
hmax: float = None, | ||
nprocs: int = None, | ||
verbosity: int = 0, | ||
method: str = "exact", | ||
base_as_hfun: bool = True, | ||
base_shape: Union[Polygon, MultiPolygon] = None, | ||
base_shape_crs: Union[str, CRS] = "EPSG:4326", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation alignement
self, | ||
value, | ||
upper_bound=np.inf, | ||
lower_bound=-np.inf, | ||
value_type: str = "min", | ||
rate=0.01, | ||
source_index: Union[List[int], int, None] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation alignement
self, | ||
func=lambda i: i / 2.0, | ||
upper_bound=np.inf, | ||
lower_bound=-np.inf, | ||
value_type: str = "min", | ||
rate=0.01, | ||
source_index: Union[List[int], int, None] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation alignement
self, | ||
level: Union[List[float], float] = None, | ||
expansion_rate: float = 0.01, | ||
target_size: float = None, | ||
contour_defn: Contour = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation alignment with body
self, | ||
level: float = 0, | ||
width: float = 1000, # in meters | ||
target_size: float = 200, | ||
expansion_rate: float = None, | ||
tolerance: Union[None, float] = 50, | ||
channel_defn=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation alignment
# self._validate_raster_local( | ||
# Raster(target_path), tmpraster.md5 | ||
# os.copyfile() | ||
# tgtraster.save(target_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space after #. note not to remove code indentation
from ocsmesh.mesh.base import BaseMesh | ||
from ocsmesh.mesh.parsers import grd, sms2dm | ||
from ocsmesh.raster import Raster | ||
from pyproj import CRS, Transformer | ||
from scipy.interpolate import RectBivariateSpline, RegularGridInterpolator | ||
from shapely.geometry import LineString, MultiPolygon, Polygon, box | ||
from shapely.ops import linemerge, polygonize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate third party from ocsmesh
self, | ||
threshold=0.0, | ||
land_ibtype=0, | ||
interior_ibtype=1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
body argument alignment
# ocean_boundaries = utils.sort_edges(ocean_boundary) | ||
# land_boundaries = utils.sort_edges(land_boundary) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra spaces
self, | ||
path: Union[str, os.PathLike], | ||
overwrite: bool = False, | ||
format="grd", # pylint: disable=W0622 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
body arg alignment
self, | ||
dem_files: Union[None, Sequence[Union[str, os.PathLike]]], | ||
out_file: Union[str, os.PathLike], | ||
out_format: str = "shapefile", | ||
mesh_file: Union[str, os.PathLike, None] = None, | ||
mesh_multipolygon: Union[MultiPolygon, Polygon] = None, | ||
ignore_mesh_final_boundary: bool = False, | ||
zmin: Union[float, None] = None, | ||
zmax: Union[float, None] = None, | ||
chunk_size: Union[int, None] = None, | ||
overlap: Union[int, None] = None, | ||
nprocs: int = -1, | ||
out_crs: Union[str, CRS] = "EPSG:4326", | ||
base_crs: Union[str, CRS] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argument body alignment
# priority_args = [] | ||
# for priority, dem_file in zip(priorities, dem_files): | ||
# priority_args.append( | ||
# (priority, temp_dir, dem_file, chunk_size, overlap)) | ||
# | ||
# with Pool(processes=nprocs) as p: | ||
# p.starmap(self._process_priority, priority_args) | ||
# p.join() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra space due to shift
self, | ||
path: Union[str, os.PathLike], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alignment thing
self, | ||
priority: int, | ||
temp_dir: Union[str, os.PathLike], | ||
dem_path: Union[str, os.PathLike], | ||
chunk_size: Union[int, None] = None, | ||
overlap: Union[int, None] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alignment
self, | ||
base_mesh_path: Union[str, os.PathLike, None], | ||
temp_dir: Union[str, os.PathLike], | ||
priorities: Sequence[int], | ||
dem_files: Sequence[Union[str, os.PathLike]], | ||
z_info: dict = {}, | ||
chunk_size: Union[int, None] = None, | ||
overlap: Union[int, None] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alignment
# priority_geodf = gpd.GeoDataFrame( | ||
# columns=['geometry'], | ||
# crs=self._calc_crs) | ||
# for p in range(priority): | ||
# higher_pri_path = ( | ||
# pathlib.Path(temp_dir) / f'dem_priority_{p}.feather') | ||
# | ||
# if higher_pri_path.is_file(): | ||
# priority_geodf = priority_geodf.append( | ||
# self._read_to_geodf(higher_pri_path)) | ||
# | ||
# if len(priority_geodf): | ||
# op_res = priority_geodf.unary_union | ||
# pri_mult_poly = MultiPolygon() | ||
# if isinstance(op_res, MultiPolygon): | ||
# pri_mult_poly = op_res | ||
# else: | ||
# pri_mult_poly = MultiPolygon([op_res]) | ||
# | ||
# | ||
# if rast_box.within(pri_mult_poly): | ||
# _logger.info( | ||
# f"{dem_path} is ignored due to priority...") | ||
# continue | ||
# | ||
# if rast_box.intersects(pri_mult_poly): | ||
# _logger.info( | ||
# f"{dem_path} needs clipping by priority...") | ||
# | ||
# # Clipping raster can cause problem at | ||
# # boundaries due to difference in pixel size | ||
# # between high and low resolution rasters | ||
# # so instead we operate on extracted polygons | ||
# geom_mult_poly = geom_mult_poly.difference( | ||
# pri_mult_poly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra spaces due to shift
self, | ||
base_mesh_path: Union[str, os.PathLike, None], | ||
temp_dir: Union[str, os.PathLike], | ||
priority: int, | ||
dem_file: Union[str, os.PathLike], | ||
z_info: dict = {}, | ||
chunk_size: Union[int, None] = None, | ||
overlap: Union[int, None] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alignment
self, | ||
dem_files: Sequence[Union[str, os.PathLike]], | ||
out_file: Union[str, os.PathLike], | ||
out_format: str = "shapefile", | ||
mesh_file: Union[str, os.PathLike, None] = None, | ||
hmin: Union[float, None] = None, | ||
hmax: Union[float, None] = None, | ||
contours: List[List[float]] = None, | ||
constants: List[List[float]] = None, | ||
chunk_size: Union[int, None] = None, | ||
overlap: Union[int, None] = None, | ||
method: str = "exact", | ||
nprocs: int = -1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arg body alignment
self, | ||
path: Union[str, os.PathLike], | ||
crs: Union[str, CRS] = None, | ||
chunk_size=None, | ||
overlap=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
body arg alignment
self, | ||
hmin=None, | ||
zmin=None, | ||
zmax=None, | ||
window=None, | ||
overlap=None, | ||
band=1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
body arg alignment
self, | ||
band=1, | ||
window=None, | ||
axes=None, | ||
vmin=None, | ||
vmax=None, | ||
cmap="topobathy", | ||
levels=None, | ||
show=False, | ||
title=None, | ||
figsize=None, | ||
colors=256, | ||
cbar_label=None, | ||
norm=None, | ||
**kwargs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
body arg alignment
# # Write orignal band | ||
# dst.write_band(i + n_bands_new // 2, outband) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra spaces due to shift
self, | ||
level: float = 0, | ||
width: float = 1000, # in meters | ||
tolerance: Union[None, float] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
body arg alignment
mesh: jigsaw_msh_t, | ||
shape: Union[box, Polygon, MultiPolygon], | ||
from_box: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
body arg alignment
mesh: jigsaw_msh_t, | ||
shape: Union[box, Polygon, MultiPolygon], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
body arg alignment
mesh: jigsaw_msh_t, | ||
shape: Union[box, Polygon, MultiPolygon], | ||
use_box_only: bool = False, | ||
fit_inside: bool = True, | ||
inverse: bool = False, | ||
in_place: bool = False, | ||
check_cross_edges: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argument body alignment
mesh: jigsaw_msh_t, | ||
vert_in: Sequence[int], | ||
can_use_other_verts: bool = False, | ||
inverse: bool = False, | ||
in_place: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argument body alignment
*mesh_list, | ||
out_crs="EPSG:4326", | ||
drop_by_bbox=True, | ||
can_overlap=True, | ||
check_cross_edges=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argument body alignment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @yosoyjay, thank you for the changes. It looks much cleaner. I just have a couple of concerns that is vividly visible from my comments on the code:
- Some function/method arguments are indented such that the indentation aligns with the body
- Some commented lines are shifted and the commented code are not indented properly and there's extra space
- Some calls spread the arguments (1 per line) and sometimes it's too much.
- Some lines seem to have become very long.
Otherwise I really liked these changes. It would be useful to add it as Github actions so that it's also clear how to run checks locally before submitting the code to repo.
I also have another concern. I made a lot of changes for adding docstring in enhance/docstring
branch. I think there would be a lot of conflicts for merging, so I was wondering how much of your changes are manual and how much automatic. In case most things are automatic, we can first merge docstring branch to main and then apply changes to that branch. I'll address your questions in a separate comment.
For the question of those specific errors (line numbers are based on files in the pull request branch):
|
This pull request is to improve the linting for the package in response to issue #105 in ODSSM.
The current PR includes changes resulting the application of
isort
andblack
and small change insetup.py
to improve the robustness of the version check for the C++ complier (-dumpversion
vs.--version
) .flake8
compliance is a work in progress.flake8
is currently configured insetup.cfg
to ignore E501 (line length), E226 (lack of white space around binary operators), and E503 (line break before binary operator, but other errors remain that I'd like feedback on:Once the
flake8
issue is resolved, I'd be in favor of integrating tests forisort
,black
, andflake8
as part of the Github Action workflow for testing.