search

schism-dev / pyschism

Python interface for handling the SCHISM model.
https://schism-dev.github.io/schism/master/getting-started/pre-processing-with-pyschism/overview.html
Apache License 2.0
24 stars 20 forks source link

`constituents` input to `write` method of `Bctides` is unused #102

Closed SorooshMani-NOAA closed 11 months ago

SorooshMani-NOAA commented 11 months ago

In the new API for BCTides, the input for constituents is taken (and set to Tides) in the __init__ of Bctides class. https://github.com/schism-dev/pyschism/blob/f5fa0c26b518632414e033fe3d78d46bb75d891d/pyschism/forcing/bctides/bctides.py#L11-L21 https://github.com/schism-dev/pyschism/blob/f5fa0c26b518632414e033fe3d78d46bb75d891d/pyschism/forcing/bctides/bctides.py#L54

However the write method again gets the value of constituents and sets it to the self.constituents which seems to be unused: https://github.com/schism-dev/pyschism/blob/f5fa0c26b518632414e033fe3d78d46bb75d891d/pyschism/forcing/bctides/bctides.py#L231-L236 https://github.com/schism-dev/pyschism/blob/f5fa0c26b518632414e033fe3d78d46bb75d891d/pyschism/forcing/bctides/bctides.py#L259-L262

I think the constituents argument for write method needs to be removed as it is misleading and unused as the constituents are already accounted for in the Tides object creation and active ones are taken from there: https://github.com/schism-dev/pyschism/blob/f5fa0c26b518632414e033fe3d78d46bb75d891d/pyschism/forcing/bctides/bctides.py#L276-L282 .https://github.com/schism-dev/pyschism/blob/f5fa0c26b518632414e033fe3d78d46bb75d891d/pyschism/forcing/bctides/bctides.py#L74 https://github.com/schism-dev/pyschism/blob/f5fa0c26b518632414e033fe3d78d46bb75d891d/pyschism/forcing/bctides/bctides.py#L92 https://github.com/schism-dev/pyschism/blob/f5fa0c26b518632414e033fe3d78d46bb75d891d/pyschism/forcing/bctides/bctides.py#L159

cuill commented 11 months ago

@SorooshMani-NOAA Thanks for reviewing the code. I'll modify the code accordingly.

SorooshMani-NOAA commented 11 months ago

@cuill no problem! Can you please also release a new version on PyPI either before or after this update? Thanks!

cuill commented 11 months ago

@SorooshMani-NOAA Sure. Will release a new version after updating this.

SorooshMani-NOAA commented 11 months ago

Since you merged the updates related to this I'm closing this one.