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

Fix wrong number of mesh levels when grid is multiple of refinement factor #26

Merged
merged 3 commits into from
Oct 14, 2024

Conversation

joeloskarsson
Copy link
Contributor

@joeloskarsson joeloskarsson commented Sep 26, 2024

Describe your changes

There is currently a bug where you can not make hierarchical graphs if the grid dimensions are an exact multiple of the refinement factor. This is best illustrated with an example:

  • If you have a 27 x 27 grid
  • grid_refinement_factor=3 and level_refinement_factor=3

Then you should be able to have a first mesh level of 9x9 and a second of 3x3 mesh nodes. This does currently not work, as the code return that the maximum number of possible mesh levels is 1, rather than 2.

We tracked down this issue to the casting to int in

max_mesh_levels = (
(np.log(coord_extent) - np.log(grid_refinement_factor))
/ np.log(level_refinement_factor)
).astype(
int
) # (2,)

Due to the numerical precision of this calculation it sometimes ends up with something like 1.999999999999996 rather than exactly 2. This then gets floored to 1.

This PR fixes this by adding a tiny epsilon before doing this rounding. It also adds a test for this.

Type of change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📖 Documentation (Addition or improvements to documentation)

Checklist before requesting a review

  • My branch is up-to-date with the target branch - if not update your fork with the changes from the target branch (use pull with --rebase option if possible).
  • I have performed a self-review of my code
  • For any new/modified functions/classes I have added docstrings that clearly describe its purpose, expected inputs and returned values
  • I have placed in-line comments to clarify the intent of any hard-to-understand passages of my code
  • I have updated the documentation to cover introduced code changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have given the PR a name that clearly describes the change, written in imperative form (context).
  • I have requested a reviewer and an assignee (assignee is responsible for merging)

Checklist for reviewers

Each PR comes with its own improvements and flaws. The reviewer should check the following:

  • the code is readable
  • the code is well tested
  • the code is documented (including return types and parameters)
  • the code is easy to maintain

Author checklist after completed review

  • I have added a line to the CHANGELOG describing this change, in a section
    reflecting type of change (add section where missing):
    • added: when you have added new functionality
    • changed: when default behaviour of the code has been changed
    • fixes: when your contribution fixes a bug

Checklist for assignee

  • PR is up to date with the base branch
  • the tests pass
  • author has added an entry to the changelog (and designated the change as added, changed or fixed)
  • Once the PR is ready to be merged, squash commits and merge the PR.

Copy link
Contributor

@ErikLarssonDev ErikLarssonDev left a comment

Choose a reason for hiding this comment

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

Nice work!

joeloskarsson added a commit to joeloskarsson/weather-model-graphs that referenced this pull request Oct 13, 2024
@sadamov sadamov self-requested a review October 13, 2024 20:09
Copy link
Contributor

@sadamov sadamov left a comment

Choose a reason for hiding this comment

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

This looks good to me. Easy fix for numerical issues. Alternatively this also solves the isssue: coord_extent = np.array((xy.shape[2], xy.shape[1]), dtype="float32")
But, I am fine with the epsilon approach and from my side this PR can be merged.

@joeloskarsson
Copy link
Contributor Author

Thanks, that is good to know! That might mean that we no longer need the epsilon in #32. Since coord_extent is being changed in #32 I am thinking that we just merge this as a fix for now and then I'll take another look if the epsilon is still needed after that change is done.

@joeloskarsson joeloskarsson merged commit 5550c53 into mllam:main Oct 14, 2024
3 checks passed
@SimonKamuk SimonKamuk mentioned this pull request Nov 20, 2024
20 tasks
joeloskarsson added a commit to joeloskarsson/weather-model-graphs that referenced this pull request Nov 20, 2024
joeloskarsson added a commit that referenced this pull request Nov 20, 2024
* Update changelog and version number for version 0.2.0

* Move #26 to fixed in changelog
joeloskarsson added a commit that referenced this pull request Nov 29, 2024
* Start looking into where xy has to be changed

* Change shapes in docstrings

* Make flat mesh graphs work with new coordinate layout

* Merge PR #26 into branch

* Fix coordinate handling in multirange graph creation

* Rename grid_refinement_factor to mesh_node_distance

* Fix existing tests to work with new coordinate format

* Add test for irregularlygridded coordinates

* Remove unneeded eps in mesh level calculation

* Change documentation to use new format and arguments for coordinates

* Fix bug in coordinate order for flat graphs

* Start working on allowing latlon coordinates

* Introduce coords and projection

* Fix linting

* Fix tests with coords keyword argument

* Implement lat-lon transformation through projection

* Add documentation page about graphs constructed using lat-lons

* Adjust coords keyword arg in docs

* Add test for lat-lon coordinates

* Fix linting of docs

* Merge main into branch

* Fix typos and clarifications as suggested from code review

Co-authored-by: sadamov <45732287+sadamov@users.noreply.github.com>

* Change euclidean coordinates to Cartesian coordinates

* Update src/weather_model_graphs/create/mesh/kinds/hierarchical.py

Co-authored-by: Leif Denby <leif@denby.eu>

* Clarify comments and variable names around mesh level computation

* Add check for number of nodes in test with irregular coords

* Update docs line on square meshes

* Reference lat-lon notebook in coordinate section

* Change projection spec to use pyproj crs:s

* Adjust test to crs arguments

* Fix linting

* Update docs to crs change

* Add cartopy dependency to visualization group

* Update src/weather_model_graphs/create/base.py

Co-authored-by: Leif Denby <leif@denby.eu>

* Fix documentation

* Add changelog entry

* Update CHANGELOG.md

Co-authored-by: Leif Denby <leif@denby.eu>

* Trim whitespace

---------

Co-authored-by: sadamov <45732287+sadamov@users.noreply.github.com>
Co-authored-by: Leif Denby <leif@denby.eu>
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