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

Enhance (and rename) nicer_slicer -> longitude_slicer #112

Merged
merged 17 commits into from
Mar 13, 2024

Conversation

navidcy
Copy link
Contributor

@navidcy navidcy commented Feb 27, 2024

Closes #32

@navidcy
Copy link
Contributor Author

navidcy commented Feb 27, 2024

I noticed

## Here we assume that data has equally spaced longitude values spanning 360 degrees.

Let's add an assert to ensure our assumption is correct!

@navidcy
Copy link
Contributor Author

navidcy commented Feb 27, 2024

I also noticed

data wraps around (commonly either in domain [-180, 180] or in [-270, 90]).

Why isn't [0, 360] not part of the "commonly used domains"?

@navidcy
Copy link
Contributor Author

navidcy commented Feb 27, 2024

I think nicer_slicer should be renamed to longitude_slicer, right?

@navidcy
Copy link
Contributor Author

navidcy commented Feb 27, 2024

Question:

Here:

- Determine whether we need to add or subtract 360 to get the
middle of the ``xextent`` to lie within ``data``'s longitude range
(hereby ``oldx``).
- Shift the dataset so that its midpoint matches the midpoint of
``xextent`` (up to a muptiple of 360). Now, the modified ``oldx``
does not increase monotonically from West to East since the
'seam' has moved.
- Fix ``oldx`` to make it monotonically increasing again. This uses
the information we have about the way the dataset was
shifted/rolled.

we mention oldx but I don't see any oldx in the source code. Is the oldx only for reference in the docstring or was there an oldx variable at some point that got deprecated?

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@navidcy navidcy marked this pull request as ready for review February 29, 2024 06:27
@navidcy navidcy requested review from ashjbarnes and angus-g and removed request for ashjbarnes February 29, 2024 06:27
@navidcy
Copy link
Contributor Author

navidcy commented Feb 29, 2024

Ideally this PR should also deal with #118. I'm not sure if the demo notebook uses this slicer (and that's how my need for #119 came about).

@navidcy navidcy changed the title Enhance (and rename) nicer_slicer Enhance (and rename) nicer_slicer -> longitude_slicer Feb 29, 2024
Copy link
Collaborator

@ashjbarnes ashjbarnes left a comment

Choose a reason for hiding this comment

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

Looks good. Might need to merge renaming to longitude_slicer will merge nicely with #122

@navidcy navidcy merged commit 07ba1bd into main Mar 13, 2024
4 checks passed
@navidcy navidcy deleted the ncc-ajb/nicer_slicer branch March 13, 2024 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup 🧹 documentation 📔 Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More verbose method/function naming + push for explicit keyword args vs positional args
2 participants