Use UVParameter forms in UVData.__add__ and UVData.fast_concat to prevent missing parameters. #1606
Conversation
bhazelton
force-pushed
the
add_concat_use_axis
branch
from
e0ef552 to
829fab9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1606 +/- ##
==========================================
- Coverage 99.93% 99.93% -0.01%
==========================================
Files 67 67
Lines 22688 22670 -18
==========================================
- Hits 22674 22656 -18
Misses 14 14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@kartographer this is a slightly different approach than you took in the |
bhazelton
force-pushed
the
add_concat_use_axis
branch
3 times, most recently
from
f6b1427 to
33b50d9
Compare
bhazelton
force-pushed
the
add_concat_use_axis
branch
from
89f562f to
c604805
Compare
bhazelton
force-pushed
the
add_concat_use_axis
branch
5 times, most recently
from
945b3a6 to
34449d2
Compare
bhazelton
force-pushed
the
add_concat_use_axis
branch
from
34449d2 to
a1f87dc
Compare
bhazelton
force-pushed
the
add_concat_use_axis
branch
from
a1f87dc to
26ed06f
Compare
steven-murray
left a comment
steven-murray
left a comment
There was a problem hiding this comment.
Thanks @bhazelton and sorry for the very slow review! I have a few comments. Mostly I think we can make some of these functions a bit more clear in what they are doing. They seem like they will become quite central in how UVxxx objects operate, so we should make them as clear as possible for our own future reference.
It might also be useful to do a quick profiling for these functions, as I expect they might be bottleneck functions in some applications (especially in terms of memory)
| return np.asarray(inds) | ||
|
|
||
|
|
||
| def flt_ind_str_arr( |
There was a problem hiding this comment.
should this be flt_int_str_arr? And can we just make it float_int_to_str_array?
| flt_first: bool = True, | ||
| ) -> StrArray: | ||
| """ | ||
| Create a string array built from float and integer arrays for matching. |
There was a problem hiding this comment.
Would be good to have an example like "E.g. for float 3.7 and int 3 this will create an entry in the output array as '3.7_3'"
|
|
||
| def _get_param_axis(self, axis_name: str, single_named_axis: bool = False): | ||
| """ | ||
| Get a mapping of parameters that have a given axis to the axis number. |
There was a problem hiding this comment.
Can we use "attributes" instead of "parameters" here? I was confused what this function was doing until I realized you were talking about attributes.
Probably this will be even more easily cleared up by including a single simple example.
| self, | ||
| other, | ||
| axis_name: str, | ||
| other_inds: IntArray, |
There was a problem hiding this comment.
should this also have a default of None, corresponding to including all entries from the other object?
| new_array = np.concatenate( | ||
| [ | ||
| getattr(self, param), | ||
| getattr(other, "_" + param).get_from_form(other_form_dict), |
There was a problem hiding this comment.
Is there a reason we couldn't just use np.take(getattr(other, param), other_inds, axis) here?
| if param not in multi_axis_params: | ||
| continue |
There was a problem hiding this comment.
Why are we not also padding single-axis objects? This is confusing
| order_dict : dict | ||
| dict giving the final sort indices for each axis (keys are axes, values | ||
| are index arrays for sorting). |
There was a problem hiding this comment.
Since this sorting occurs in multiple functions, might it be better to just have a standalone "arbitrary axis sorting helper"?
Description
This is following some of the ideas in UVBase._select_along_axis. This could probably be generalized to UVBase, but I wanted to get UVData's version done to work out the ideas before moving it up UVBase. And I wanted to at least get a draft up for @rlbyrne to try out.
Motivation and Context
fixes #1595
Types of changes
Checklist:
Bug fix checklist: