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

Black Formatting #281

Merged
merged 66 commits into from
Feb 1, 2024
Merged

Black Formatting #281

merged 66 commits into from
Feb 1, 2024

Conversation

ssolson
Copy link
Contributor

@ssolson ssolson commented Nov 29, 2023

This PR adds starts to address formatting issues in MHKiT by adding black:

This PR adds black because it will automatically format our files allowing our users to focus on coding and letting black fix the formatting issues automatically.

To adhere to black formatting developers can install and run black from the terminal manually, they can install black as an extension in their IDE such as VS code, or they can enable enable the pre-commit hook.

What this PR changes:

  • Ran black . formatting on the repo
    • After installing black I auto reformatted the repository using the above command
  • add an GitHub actions check that code adheres to black
  • Added an optional pre commit that will auto format files to adhere to black on commits should a developer choose to use it.
  • Added test coverage so that tests would pass

@ssolson ssolson linked an issue Nov 29, 2023 that may be closed by this pull request
@ssolson ssolson changed the title MHKiT Formatting MHKiT Black Formatting Nov 29, 2023
@ssolson ssolson changed the title MHKiT Black Formatting Black Formatting Nov 29, 2023
@ssolson ssolson self-assigned this Nov 29, 2023
@ssolson ssolson mentioned this pull request Nov 29, 2023
@ssolson ssolson marked this pull request as ready for review January 31, 2024 03:18
@ssolson
Copy link
Contributor Author

ssolson commented Jan 31, 2024

@akeeste this is ready for review

@ssolson ssolson requested a review from akeeste January 31, 2024 14:59
@ssolson ssolson added the Clean Up Improve code consistency and readability label Jan 31, 2024
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 @ssolson. This looks like a lot of good work to get this up and running, especially adding all those error handling tests.

As we've discussed, the large number of files makes this difficult to review. So my review involved:

  • reviewing readme instructions on using black
  • installing and testing black myself on new python files
  • reviewing how formatting has generally been changed in a few sample files
  • skimming changes to test files to identify where new tests have been added
  • non-test python files ignored

A couple notes here:

  • with my current MHKiT environment I had some dependency conflicts (jupyter-server and pywinpty) when installing pre-commit. I tried upgrading a those specific packages which broke others (jupyterlab, jupyterlab-server). I have not tried reinstalling everything at once yet. Did you get pre-commit installed okay with the rest of your MHKiT environment?
  • See note about commented tests in wave.test_contours

Comment on lines 480 to 544
# def test_nonparametric_copulas(self):
# methods = [
# "nonparametric_gaussian",
# "nonparametric_clayton",
# "nonparametric_gumbel",
# ]

# np_copulas = wave.contours.environmental_contours(
# self.wdrt_Hm0, self.wdrt_Te, self.wdrt_dt, self.wdrt_period, method=methods
# )

# close = []
# for method in methods:
# close.append(
# np.allclose(
# np_copulas[f"{method}_x1"],
# self.wdrt_copulas[f"{method}_x1"],
# atol=0.13,
# )
# )
# close.append(
# np.allclose(
# np_copulas[f"{method}_x2"],
# self.wdrt_copulas[f"{method}_x2"],
# atol=0.13,
# )
# )
# self.assertTrue(all(close))

# def test_kde_copulas(self):
# kde_copula = wave.contours.environmental_contours(
# self.wdrt_Hm0,
# self.wdrt_Te,
# self.wdrt_dt,
# self.wdrt_period,
# method=["bivariate_KDE"],
# bandwidth=[0.23, 0.23],
# )
# log_kde_copula = wave.contours.environmental_contours(
# self.wdrt_Hm0,
# self.wdrt_Te,
# self.wdrt_dt,
# self.wdrt_period,
# method=["bivariate_KDE_log"],
# bandwidth=[0.02, 0.11],
# )

# close = [
# np.allclose(
# kde_copula["bivariate_KDE_x1"], self.wdrt_copulas["bivariate_KDE_x1"]
# ),
# np.allclose(
# kde_copula["bivariate_KDE_x2"], self.wdrt_copulas["bivariate_KDE_x2"]
# ),
# np.allclose(
# log_kde_copula["bivariate_KDE_log_x1"],
# self.wdrt_copulas["bivariate_KDE_log_x1"],
# ),
# np.allclose(
# log_kde_copula["bivariate_KDE_log_x2"],
# self.wdrt_copulas["bivariate_KDE_log_x2"],
# ),
# ]
# self.assertTrue(all(close))

Copy link
Contributor

Choose a reason for hiding this comment

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

should these tests still be commented? Let's remove or add back in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good Catch. These should be added back. They are a significant source of time during testing.

@akeeste
Copy link
Contributor

akeeste commented Feb 1, 2024

Noting here some non-black related changes, @ssolson add anything else I've missed that you did. We've largely discussed all of these and they aren't issues, but noting for future reference:

  • new API key added for NREL's HSDS server
  • mhkit.tests.river.test_io broken out into mhkit.tests.river.test_io_d3d and mhkit.tests.river.test_io_usgs
  • added functional tests and error handing tests to mhkit.tests.dolfyn.test_tools, mhkit.tests.mooring.test_mooring
  • added error handling tests for mhkit.tests.river.test_resource, mhkit.tests.wave.io.hindcast.test_wind_toolkit, mhkit.tests.wave.test_contours, mhkit.tests.loads.test_loads

@ssolson
Copy link
Contributor Author

ssolson commented Feb 1, 2024

Noting here some non-black related changes, @ssolson add anything else I've missed that you did. We've largely discussed all of these and they aren't issues, but noting for future reference:

  • new API key added for NREL's HSDS server
  • mhkit.tests.river.test_io broken out into mhkit.tests.river.test_io_d3d and mhkit.tests.river.test_io_usgs
  • added functional tests and error handing tests to mhkit.tests.dolfyn.test_tools, mhkit.tests.mooring.test_mooring
  • added error handling tests for mhkit.tests.river.test_resource, mhkit.tests.wave.io.hindcast.test_wind_toolkit, mhkit.tests.wave.test_contours, mhkit.tests.loads.test_loads

Thank you for documenting all these.

@ssolson
Copy link
Contributor Author

ssolson commented Feb 1, 2024

  • with my current MHKiT environment I had some dependency conflicts (jupyter-server and pywinpty) when installing pre-commit. I tried upgrading a those specific packages which broke others (jupyterlab, jupyterlab-server). I have not tried reinstalling everything at once yet. Did you get pre-commit installed okay with the rest of your MHKiT environment?

Adam as I mentioned in the meeting I uninstalled the pre-commit as I found it slow.

I just reinstalled it and I did not get any conflicts.
image

@akeeste
Copy link
Contributor

akeeste commented Feb 1, 2024

  • with my current MHKiT environment I had some dependency conflicts (jupyter-server and pywinpty) when installing pre-commit. I tried upgrading a those specific packages which broke others (jupyterlab, jupyterlab-server). I have not tried reinstalling everything at once yet. Did you get pre-commit installed okay with the rest of your MHKiT environment?

Adam as I mentioned in the meeting I uninstalled the pre-commit as I found it slow.

I just reinstalled it and I did not get any conflicts. image

That sounds good, thanks for checking. I just wanted to make sure that it could work, even if its slow since we call it out as an option in the readme.

@akeeste
Copy link
Contributor

akeeste commented Feb 1, 2024

Once the commented tests are back in this should be good to go! Thanks @ssolson

@ssolson ssolson merged commit f282687 into MHKiT-Software:develop Feb 1, 2024
17 checks passed
@ssolson ssolson deleted the lint 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.

Automatic Linting
2 participants