Improved efficiency of overlap_sort #1465
Conversation
|
@caseyflex should we close this (or make a draft) in view of #1464 ? |
no sorry -- we should close this one: #1463 This PR uses the revised outer_dot PR #1464 to improve overlap_sort. |
|
ok I see, sorry for the confusion |
| ``self.monitor.mode_spec`` and ``self.monitor.freqs`` will no longer be matching the | ||
| newly created data.""" | ||
|
|
||
| update_dict = dict(self._grid_correction_dict, **self.field_components) |
There was a problem hiding this comment.
_grid_correction_dict is a property so it shouldn't be added here at all?
There was a problem hiding this comment.
I guess this is adding grid_primal_correction and grid_dual_correction to the update list. I copied this from _isel, but it is probably not needed as these fields keep their old values...
There was a problem hiding this comment.
oh actually, it does do something -- it updates the coordinates on those two fields as well as the field_components. This may actually be needed so best to leave it.
| @@ -1081,6 +1081,7 @@ def overlap_sort( | |||
| for freq_id in range(f0_ind + step, last_ind, step): | |||
| # Get next frequency to sort | |||
| data_to_sort = self._isel(f=[freq_id]) | |||
| data_to_sort = data_to_sort._assign_coords(f=[self.monitor.freqs[f0_ind]]) | |||
There was a problem hiding this comment.
I am confused, why is this needed? For a regular DataArray, isel-ing already redefines the coordinates, too. Is something not working right in our _isel wrapper?
There was a problem hiding this comment.
It's a bit of a hack -- I want to isel f=[freq_id], which is the next frequency to sort. But, I want to compute outer_dot against the base frequency f0. But because of how outer_dot works, it only keeps frequencies which are common between the two datasets. So, here, I isel f=[freq_id], then I assign coords to f0, so that outer_dot will think they are at the same frequency and know to compare them.
There was a problem hiding this comment.
Ohhhhh. Funny. One thing we could do also do is to allow outer_dot to work without a frequency dimension, and just pop it? But, eh, I think this is fine.
caseyflex
force-pushed
the
casey/alternativeouterdot
branch
from
e498b81
to
e5297f7
Compare
caseyflex
force-pushed
the
casey/modesorting
branch
from
9a1eee1
to
a9117db
Compare
caseyflex
force-pushed
the
casey/alternativeouterdot
branch
from
e5297f7
to
64e01fa
Compare
caseyflex
force-pushed
the
casey/modesorting
branch
from
a9117db
to
b9ad790
Compare
|
I don't know if @weiliangjin2021 still wants to look at this, but it should be good to merge after a rebase. The conflicts are probably in the changelog. You could squash the commits too. |
caseyflex
force-pushed
the
casey/modesorting
branch
from
b9ad790
to
a6a90c1
Compare
Rebased and squashed. Merge when you're ready. thanks! |
Uses the faster outer_dot to improve efficiency of overlap_sort. Sometimes, sorting modes can be a bottleneck in mode solving, and this should prevent that from occuring.
Based off of #1464