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

Bounds not being calculated properly when crop=True #356

Closed
robbibt opened this issue Oct 30, 2024 · 3 comments · Fixed by #357
Closed

Bounds not being calculated properly when crop=True #356

robbibt opened this issue Oct 30, 2024 · 3 comments · Fixed by #357

Comments

@robbibt
Copy link
Contributor

robbibt commented Oct 30, 2024

While looking into #355, I noticed that crop=True didn't seem to be having any effect on run-times, unless bounds=[...] was also provided.

I have a feeling that the default bounds value here isn't being set correctly:
https://github.com/tsutterley/pyTMD/blob/main/pyTMD/io/FES.py#L230

I believe the kwargs.setdefault('bounds', [xmin, xmax, ymin, ymax]) call in io.FES.extract_constants is meant to return [xmin, xmax, ymin, ymax] if no bounds are provided. However, this default is only set if bounds is absent from kwargs completely. The issue is that bounds does exist in kwargs: it has a default value set to None here:
https://github.com/tsutterley/pyTMD/blob/main/pyTMD/io/model.py#L1001

>>> print(kwargs)
{'version': 'EOT20', 'scale': 0.01, 'compressed': False, 'type': 'z', 'crop': True, 'bounds': None, 'method': 'spline', 'extrapolate': True, 'cutoff': inf, 'append_node': False, 'apply_flexure': False}

So because bounds does exist in kwargs, the default bounds value is never set to [xmin, xmax, ymin, ymax] and remains None - which means this line never triggers:
https://github.com/tsutterley/pyTMD/blob/main/pyTMD/io/FES.py#L258

>>> print(kwargs['crop'] and np.any(kwargs['bounds']))
None

I've only tested io.FES.extract_constants on the latest pyTMD==2.1.7; not sure if this affects io.FES.*constants or the other io modules.

tsutterley added a commit that referenced this issue Oct 30, 2024
@tsutterley
Copy link
Owner

thanks @robbibt! fix is in #357

@robbibt
Copy link
Contributor Author

robbibt commented Oct 31, 2024

Hey @tsutterley, thanks for the quick fix! This solves my problem, but it looks like there's still issues when setting crop=True via from pyTMD.compute import tide_elevations: bounds is passed as None here, and so it still ends up propagating down to io.FES.extract_constants and overriding kwargs.setdefault('bounds', [xmin, xmax, ymin, ymax]):
https://github.com/tsutterley/pyTMD/blob/main/pyTMD/compute.py#L362

{'version': 'FES2022', 'compressed': False, 'type': 'z', 'crop': True, 'bounds': None, 'method': 'spline', 'extrapolate': True, 'cutoff': inf, 'append_node': False, 'apply_flexure': False, 'scale': 0.01}

**kwargs possibly might need to be passed here too:
https://github.com/tsutterley/pyTMD/blob/main/pyTMD/compute.py#L361-L364
https://github.com/tsutterley/pyTMD/blob/main/pyTMD/compute.py#L232

tsutterley added a commit that referenced this issue Oct 31, 2024
fix: correct error when using default bounds in `extract_constants` for #356
fix: correct `TPXO10-atlas-v2` binary grid filename
refactor: use `setuptools` entry points for scripts
tsutterley added a commit that referenced this issue Oct 31, 2024
fix: correct error when using default bounds in `extract_constants` for #356
fix: correct `TPXO10-atlas-v2` binary grid filename
@tsutterley
Copy link
Owner

hopefully second time is the charm 😅

tsutterley added a commit that referenced this issue Nov 1, 2024
fix: correct error when using default bounds in `extract_constants` for #356
fix: correct `TPXO10-atlas-v2` binary grid filename for #358
fix: some ce1973 table entries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants