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

Lint Loads #288

Merged
merged 98 commits into from
Mar 13, 2024
Merged

Lint Loads #288

merged 98 commits into from
Mar 13, 2024

Conversation

ssolson
Copy link
Contributor

@ssolson ssolson commented Feb 5, 2024

This PR is part of an ongoing effort to enhance the MHKiT codebase's quality and maintainability, specifically targeting the loads module. Our main objectives were to ensure Pylint compliance and to enrich the code with type hints across all functions within this module. Here's a summary of the key changes and considerations:

Pylint Compliance

Revised the loads module to adhere to Pylint's coding standards. This involved a comprehensive review and subsequent adjustments to formatting, naming conventions, and structure to meet the stringent requirements set forth by Pylint.

A new Pylint test can be seen on this PR that enforces Pylint compliance on just the loads module.

Type Hints Addition

Type hints have been added to each function within the module, providing clearer documentation and facilitating better IDE support for type checking. This enhancement is expected to improve code readability and maintainability significantly.

Exceptions & Disables

While striving for Pylint compliance, we've encountered a few instances where adhering strictly to Pylint's recommendations would either compromise the codebase's integrity or cause excessive delays. In these cases, we've opted to disable specific Pylint checks, detailed as follows:

  • Excessive Arguments: For functions flagged with R0913 (too many arguments) and R0914 (too many local variables), we've allowed exceptions to maintain the current functionality and interface.
  • Special Case Variable n: In the LongTermExtreme function, the variable name n is retained with a disable directive to avoid unforeseen impacts on the code due to renaming.
  • Incorrect Warning W0632: Pylint mistakenly flagged a function for returning fewer arguments than expected. We've disabled this check after thorough verification.
  • Method Argument Conflict: The _Peaks class's cdf method required a disable for arguments-differ to resolve a conflict with the built-in rv_continuous method's signature.

ssolson and others added 30 commits August 31, 2023 07:11
@ssolson ssolson marked this pull request as ready for review February 23, 2024 19:39
@ssolson ssolson requested a review from akeeste February 23, 2024 19:39
@ssolson
Copy link
Contributor Author

ssolson commented Feb 23, 2024

@akeeste this should be ready for a review. I pushed really hard to get this finished before being OOO next week. I hope I didn't miss anything but I believe I have all my bases covered for a review.

@akeeste
Copy link
Contributor

akeeste commented Mar 8, 2024

sorry for the wait here, I finally got #285 ironed out and ready to go. I blocked time to review this PR first thing next week.

Copy link
Contributor

@akeeste akeeste left a comment

Choose a reason for hiding this comment

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

thanks for this PR @ssolson. Overall I think these pylint changes look pretty good, though it's hard to review the loads.extreme since it was broken into multiple files. Is there any specific changes to review for those functions?

I think your to_numeric_array function is actually a good complement to the utils.data_utils functions that convert function input to xarray format. There is some overlap, but I also see use cases for both right now--especially in graphics functions that require a more simplified numeric format, or function like those in loads.extreme.extremes that were previously built to only take numpy arrays.

@ssolson
Copy link
Contributor Author

ssolson commented Mar 11, 2024

thanks for this PR @ssolson. Overall I think these pylint changes look pretty good, though it's hard to review the loads.extreme since it was broken into multiple files. Is there any specific changes to review for those functions?

I think your to_numeric_array function is actually a good complement to the utils.data_utils functions that convert function input to xarray format. There is some overlap, but I also see use cases for both right now--especially in graphics functions that require a more simplified numeric format, or function like those in loads.extreme.extremes that were previously built to only take numpy arrays.

Adam thanks for the review. I don't have anything specific to check. I was required to breakout the functions into multiple files because the file was over 1000 lines long which throws a pylint error.

@akeeste
Copy link
Contributor

akeeste commented Mar 11, 2024

@ssolson That makes sense. I'll assume that any other line changes were similar to those done in other loads submodules. Since the tests are passing, I'm assuming all those changes are okay.

The only outstanding thing here is to decide if we can consolidate type handling functions. Let me know what you think after reviewing #285. I support merging both this and #285, then updating/standardizing type handling in the loads module in a new small PR.

@ssolson
Copy link
Contributor Author

ssolson commented Mar 13, 2024

@ssolson That makes sense. I'll assume that any other line changes were similar to those done in other loads submodules. Since the tests are passing, I'm assuming all those changes are okay.

The only outstanding thing here is to decide if we can consolidate type handling functions. Let me know what you think after reviewing #285. I support merging both this and #285, then updating/standardizing type handling in the loads module in a new small PR.

@akeeste I think there are a couple of things to address on #285 so lets get this PR merged into develop and I think you should move your 2 util functions into type_handling vs the current data_utils as discussed in my review.

@akeeste
Copy link
Contributor

akeeste commented Mar 13, 2024

@ssolson agreed, type_handling is a better name. Merging!

@akeeste akeeste merged commit 09add83 into MHKiT-Software:develop Mar 13, 2024
15 checks passed
@ssolson ssolson mentioned this pull request Mar 19, 2024
@ssolson ssolson deleted the loads branch April 16, 2024 14:55
@ssolson ssolson mentioned this pull request Apr 24, 2024
@ssolson ssolson mentioned this pull request May 6, 2024
ssolson added a commit that referenced this pull request May 8, 2024
# MHKiT v0.8.0
We're excited to announce the release of MHKiT v0.8.0, which brings a host of new features, enhancements, and bug fixes across various modules, ensuring compatibility with Python 3.10 and 3.11, and introducing full xarray support for more flexible data handling. Significant updates in the Wave and DOLfYN modules improve functionality and extend capabilities.

## Python 3.10 & 3.11 Support
MHKiT now supports python 3.10 and 3.11. Support for 3.12 will follow in the next minor update.
- #240


## Wave Module
### Enhancements:
**Automatic Threshold Calculation for Peaks-Over-Threshold**: We've introduced a new feature that automatically calculates the "best" threshold for identifying significant wave events. This method, originally developed by Neary, V. S., et al. in their 2020 study, has now been translated from Matlab to Python, enhancing our existing peaks-over-threshold functionality.

**Wave Heights Analysis**: A new function, `wave_heights`, has been added to extract the heights of individual waves from a time series. This function uses zero up-crossing analysis to accurately measure wave heights, improving upon our previous methods which only provided the maximum value between up-crossings.

**Enhanced Zero Crossing Analysis**: Building on the above, the zero crossing code previously embedded in `global_peaks` has been isolated into a helper function. This modular approach not only refines the codebase but also supports new functionalities such as calculating wave heights, zero crossing periods, and identifying crests.

### Bug Fixes:
**Contour Sampling Error in Wave Contours**: A bug identified in `mhkit.wave.contours.samples_contour` has been resolved. The issue occurred when period samples defined using the maximum period resulted in values outside the interpolation range of the contour data. This was corrected by ensuring that all sampling points are within the interpolation range and adjusting the contour data selection process accordingly.

- #268 
- #252 
- #278


## Xarray Support
MHKiT functions now fully support the use of xarray for passing and returning data.

- #279 
- #282
- #285
- #302
- #310


## DOLfYN

Thanks to the many user contributions and users of MHKiT the DOLFYN module include a significant number of enhancements and bug fixes. 

### Enhancements:
**Altimeter Support**: Enhanced the Nortek Signature Reader to add capability for reading ADCP dual profile configurations.

**Data Handling Improvements**: Introduced logic to skip messy header data that can accumulate during measurements collected via Nortek software on PCs and Macs.

**Instrument Noise Subtraction**: Added a function to subtract instrument noise from turbulence intensity estimation using RMS calculations, providing results that differ by approximately 1% from the existing standard deviation-based "I" property.

**Improved File Handling**: Updates for RDI files to handle changing "number of cells" and variable "cell sizes," which are now bin-averaged into the largest cell size.

### Bug Fixes:
**Power Spectra Calculation**: Fixed a bug where a given noise value was not being subtracted from the power spectra, and noise was inadvertently added as an input to dissipation rate calculations.

**Improved Header Handling**: Allowed RDI reader to skip junk headers effectively.

**Nortek Reader C Types Update**: Adjusted C types in the Nortek reader to handle below-zero water temperatures and to allow pitch and roll values to go negative.


- #280 
- #289
- #290
- #292
- #293
- #294
- #299


## River & Tidal: D3D
Added limits to `variable_interpolation` and added 3 array input capability to `create_points`
- #271

## Developer Experience
### Black formatting
Black formatting is now enforced on all MHKiT files. This ensures consistent formatting across the MHKiT package.
- #281

### Linting & Type Hints
MHKiT is in the process of enforcing pylint and adding type hints to all functions. Currently this has been achieved and is enforced in the Loads and Power modules.
- #288 
- #296 

### CI/CD
This release introduces significant reduction in testing time for development. This is achieved by reducing the number of tests for pulls against the develop branch and only running hindcast test when changes are made to it. A bug in the hindcast CI was fixed which only ran on changes to the hindcast tests instead of the hindcast module. Additionally the wave and wind hindcast needed to be separated in 2 jobs due to the excessive time taken to run a wind cache. This created a number of follow on PRs around solidifying the logic of these job. A special case for Python 3.8, pip, and Mac OS was added to use homebrew to install NetCDF and HDF5 to get tests to pass.
- #241
- #270
- #306
- #311
- #317
- #318
- #319
- #320
- #324

### Clean Up
MHKiT fixed an implementation error where functions used assert instead of built in errors for type and value checking. Additionally these PRs removed unused files, fixed typos, and created an argument which allows users to run CDIP API calls silently.
- #276
- #272
- #273
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Up Improve code consistency and readability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants