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

Initial code for wind field reconstruction from vorticity and divergence #3266

Open
wants to merge 89 commits into
base: main
Choose a base branch
from

Conversation

winash12
Copy link

@winash12 winash12 commented Nov 8, 2023

Description Of Changes

Checklist

@winash12 winash12 changed the title Initial Code for wind field reconstruction from vorticity and divergence Initial code for wind field reconstruction from vorticity and divergence Nov 8, 2023
@winash12 winash12 marked this pull request as ready for review November 9, 2023 01:39
@winash12 winash12 requested a review from a team as a code owner November 9, 2023 01:39
@winash12 winash12 requested review from dcamron and removed request for a team November 9, 2023 01:39
@winash12 winash12 marked this pull request as draft November 16, 2023 11:54
@winash12 winash12 marked this pull request as ready for review December 6, 2023 16:10
@winash12 winash12 marked this pull request as draft January 10, 2024 08:52
@winash12
Copy link
Author

More substantively, I notice you import xarray, which I believe no other part of the MetPy codebase does. I'm not sure what the Unidata maintainers will think of that.

MetPy provides an accessor for XArray Datasets and DataArrays, so it gets used somewhere. In metpy.calc, they have decorators that turn xarray.DataArrays into pint.Quantitys on entering the function and turn the result back into a DataArray on exiting the function. Most functions therefore seem written to take and return pint.Quantity. There is a pint decorator that would convert the function inputs into specified units and provide the units for the output, so the function could be written to take and return NumPy arrays.

Thanks Prof. for your first review of this PR. Regarding the import of xarray it's not absolutely necessary. It is required to initialize these 2 data arrays
upsi = xa.zeros_like(umask) vpsi = xa.zeros_like(vmask)

Would output_u = input_vorticity * dx give the correct size and datatype? If passed instances of pint.Quantity, it should also provide the correct units.

@DWesl I am trying to fix the linting and flake8 errors this week so that I can be set up for a formal code review.

I am stuck with the flake8 and linting errors as for each error I fix and another 10 pop up. On the example code I checked in apparently cfgrib is not an engine I can use because eccodes is not used.

@DWesl
Copy link
Contributor

DWesl commented Jul 18, 2024

I am stuck with the flake8 and linting errors as for each error I fix and another 10 pop up.

Yeah, that happens sometimes. If the problem is making the changes locally then waiting for GitHub to provide test results, you can look through the workflows in .github/workflows (the file you want probably has "lint" in the name and will end with either .yml or .yaml) to see what it installs and what commands it runs after, so you can install those packages and run those commands locally until they succeed.

If the problem is it reporting lots of little nitpicky things, you could copy the new functions to a new file, run an auto-formatter on it (black is popular and checks that the code produces the same AST after it's done as before it started, autopep8, yapf, and ruff are other options), then copy the functions from the resulting file back to where you put them originally. You will want to make sure the tests still pass before committing, but this should provide a decent first pass over the code. (Running the auto-formatters on the original file is likely to change lots of other functions you're not otherwise touching, and bigger changes take longer to review).

On the example code I checked in apparently cfgrib is not an engine I can use because eccodes is not used.

I suspect that means eccodes is not installed by the existing infrastructure. You could add cfgrib/eccodes to the list of things they install, or you could convert the grib file to netCDF or CSV and use that in the example instead.

@winash12
Copy link
Author

I am stuck with the flake8 and linting errors as for each error I fix and another 10 pop up.

Yeah, that happens sometimes. If the problem is making the changes locally then waiting for GitHub to provide test results, you can look through the workflows in .github/workflows (the file you want probably has "lint" in the name and will end with either .yml or .yaml) to see what it installs and what commands it runs after, so you can install those packages and run those commands locally until they succeed.

If the problem is it reporting lots of little nitpicky things, you could copy the new functions to a new file, run an auto-formatter on it (black is popular and checks that the code produces the same AST after it's done as before it started, autopep8, yapf, and ruff are other options), then copy the functions from the resulting file back to where you put them originally. You will want to make sure the tests still pass before committing, but this should provide a decent first pass over the code. (Running the auto-formatters on the original file is likely to change lots of other functions you're not otherwise touching, and bigger changes take longer to review).

On the example code I checked in apparently cfgrib is not an engine I can use because eccodes is not used.

I suspect that means eccodes is not installed by the existing infrastructure. You could add cfgrib/eccodes to the list of things they install, or you could convert the grib file to netCDF or CSV and use that in the example instead.

Yes the lint , ruff and flake8 errors are all very nitpicky. The thing is I have pep8 and pycodestyle, pylint (not ruff I could not install it) and when I run my changes through these 3 I get no errors at all. But when it goes through MetPy I get a gazillion errors - some of them very trivial and each one I fix brings about others.

yes I will convert my GRIB file to netCDF as it's not worth modifying what MetPy will need as part of it's installation.

@DWesl
Copy link
Contributor

DWesl commented Jul 19, 2024

Interesting. My experience with pylint is it finds more things than flake8, which implies they have a few plugins. I don't remember offhand whether flake8 runs pycodestyle checks by default or only if you install a plugin, but I do remember the pep8 python package changed its name to pycodestyle a few years back to avoid confusion with PEP 8.

It looks like the linting requirements are pure python aside from ruff's dependency on Rust, which is rather less ubiquitous than the usual C, C++, or even Fortran dependencies, so installing everything between the first and last lines of the file should get you a flake8 installation that will match the one doing the checks.

For ruff, given the newness of Rust, I am somewhat surprised they do not have CI just make the changes on your PR or as a PR against the branch in your PR. You should still be able to put your functions in a new file, run black or yapf on them, then run flake8 on it until it passes (black is pure python and might have inspired ruff, I think yapf is also pure python), then copy the functions back where you got them and fix the last few flake8 issues with those files.

EDIT: On looking at the current errors, you are past most of those easy problems. The currently-flagged problems are extra spaces in and around docstrings, and writing docstring summaries in declarative rather than the imperative mood (removing the "s" at the end of the verb should fix this).

Copy link
Contributor

@DWesl DWesl left a comment

Choose a reason for hiding this comment

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

A few suggestions for the lint and coverage problems, as requested on winash12#2 here. The comments might make more sense when viewed on the "Files Changed" tab of the PR.

src/metpy/calc/tools.py Show resolved Hide resolved
Comment on lines +1991 to +1995
xindex = np.linspace(i_x_ll, i_x_ur, num=x * y, endpoint=False, dtype=np.int32)
yindex = np.linspace(i_y_ur, i_y_ll, num=y * x, endpoint=False, dtype=np.int32)
xindex = xindex.reshape((y, x), order='F')
yindex = yindex.reshape((y, x), order='C')
return [xindex, yindex]
Copy link
Contributor

Choose a reason for hiding this comment

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

What problems are you experiencing with np.mgrid and np.meshgrid that you cannot use those? They look like they do something similar.



def test_get_vectorized_array_indices():
"""Tests the vectorized array indices for two different bounding boxes. """
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Tests the vectorized array indices for two different bounding boxes. """
"""Tests the vectorized array indices for two different bounding boxes."""



def test_bounding_box_mask():
""" Test the mask of a bounding box. """
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
""" Test the mask of a bounding box. """
"""Test the mask of a bounding box."""



def test_find_bounding_box_indices():
"""Tests the bounding box indices for 2 different cases. """
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Tests the bounding box indices for 2 different cases. """
"""Tests the bounding box indices for 2 different cases."""

Comment on lines +1982 to +1983
"""Returns the vectorization indices for inner for loop in the
wind field reconstruction method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Returns the vectorization indices for inner for loop in the
wind field reconstruction method.
"""Return the vector indices for the wind field reconstruction method.

It doesn't like the "s" on the verb, and thinks the summary is too long.

Comment on lines +1964 to +1965
"""Returns the array indices of a bounding box."""

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Returns the array indices of a bounding box."""
"""Return the array indices of a bounding box."""

It wants imperative (telling the computer to do something) rather than declarative (saying what the computer does)

Comment on lines +17 to +18
vorticity, wind_components,rotational_wind_from_inversion,
divergent_wind_from_inversion)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is complaining because you started the setup for the tests but didn't add the tests for these functions. Adding the tests or removing the import will make the warning go away. Adding the tests should increase the coverage Codecov reports for the functions.

Comment on lines +74 to +76
print(crs)
vort = vorticity(u, v, longitude=lons, latitude=lats, crs=crs)

sys.exit()
Copy link
Contributor

Choose a reason for hiding this comment

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

They'll want you to change these lines back before the merge; among other things, they cause the tests to fail with a SystemExit error rather than passing.

@DWesl DWesl mentioned this pull request Aug 30, 2024
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 this pull request may close these issues.

3 participants