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

Dft reverse axis #149

Merged
merged 8 commits into from
Apr 26, 2021
Merged

Dft reverse axis #149

merged 8 commits into from
Apr 26, 2021

Conversation

lanougue
Copy link
Contributor

Handle decreasing coordinates in dft with true_phase = True.
Add a test to check

lanougue and others added 6 commits November 10, 2020 16:10
update for coming fft and ifft modifications
Moving spacing as coords attribute + some update (xgcm#126)
@lanougue
Copy link
Contributor Author

@roxyboy, merge ?

@roxyboy
Copy link
Member

roxyboy commented Apr 23, 2021

I'm personally ok to merge but I think we should wait for @rabernat 's approval as well. He should be back from vacation next week.
We should also probably update the docs page using realistic data from the xarray tutorial (e.g. #144 ).

Copy link
Collaborator

@rabernat rabernat left a comment

Choose a reason for hiding this comment

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

Thanks so much for this PR @lanougue! I really appreciate your work.

I agree with @roxyboy that documentation of this behavior is needed in the public facing docstrings and / or narrative documentation.

xrft/xrft.py Outdated
lag = coord[(len(coord.data)) // 2]
reverse_coord = (
[] if (coord[-1] > coord[0]) else [-1]
) # taking care of reverse coordinate (CAUTION : [] diff None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand this comment, specifically (CAUTION : [] diff None).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for this not being explicit. This is to emphasize that:
np.flip(a, axis=None) has a different behaviour than np.flip(a, axis=[]).
If axis=None, all axis are flipped.
If axis=[], no axis are flipped
I though it deserved to me mentioned in a comment

xrft/xrft.py Outdated
reverse_coord = (
[] if (coord[-1] > coord[0]) else [-1]
) # taking care of reverse coordinate (CAUTION : [] diff None)
lag = np.flip(coord.data, axis=reverse_coord)[(len(coord.data)) // 2]
Copy link
Collaborator

@rabernat rabernat Apr 26, 2021

Choose a reason for hiding this comment

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

For the sake of the other devs, I would prefer to make this all a bit more explicit, i.e.

if coord[-1] > coord[0]:
    coord_data = coord.data
else:
    coord_data = np.flip(coord.data, axis=-1)
lag = coord_data / (len(coord.data) // 2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok but the last line should then be:
lag = coord_data[(len(coord.data)) // 2]

@rabernat
Copy link
Collaborator

Thanks! So is the plan to defer the documentation updates to a future PR? If so, I will merge.

@roxyboy
Copy link
Member

roxyboy commented Apr 26, 2021

Thanks! So is the plan to defer the documentation updates to a future PR? If so, I will merge.

I give a thumbs up to a separate PR for docs :)

@rabernat rabernat merged commit 1d9b97f into xgcm:master Apr 26, 2021
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.

None yet

3 participants