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

Bug fix for overlap_sort() modifying self #595

Merged
merged 1 commit into from
Dec 7, 2022

Conversation

dbochkov-flexcompute
Copy link
Contributor

No description provided.

@momchil-flex
Copy link
Collaborator

My general feeling is that it should be possible to simplify some parts of the code using xarray directly to avoid things like np.exp(-1j * phase[None, None, None, :, :]) and

                field_sorted.data[..., freq_id, :] = field_sorted.data[
                    ..., freq_id, sorting[freq_id, :]
                ]

One of the big advantages of xarray is that you don't have to remember which array axis a given coordinate corresponds to (and if axes get reshuffled for some reason later on, code will still work). This is a comment not just on this PR but on the sorting code as a whole, which seems to use raw numpy a lot. However, changing this would require some thought (despite all its benefits, xarray is still not super intuitive to me) and seems like a pretty low reward / effort ratio, so I'm just leaving this comment here for the future but otherwise PR looks good!

@momchil-flex momchil-flex merged commit 05f4565 into develop Dec 7, 2022
@momchil-flex momchil-flex deleted the daniil/mode-sort-copy-bug branch December 7, 2022 01:09
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.

2 participants