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

Support more types for DLD dataframe extra columns #173

Merged
merged 1 commit into from
May 15, 2024

Conversation

philsmt
Copy link
Collaborator

@philsmt philsmt commented May 13, 2024

Building on #166, this PR relaxes the type requirements a bit to work with both pulse- and train-resolved data in pd.Series, xr.DataArray or directly KeyData objects. Naturally the pulse index must be compatible, i.e. use the same first two columns.

@philsmt philsmt force-pushed the feat/dld-flexible-extra-columns branch from 5867511 to 92c6859 Compare May 13, 2024 15:28
@@ -169,32 +172,64 @@ def _build_reduced_pd(self, data, index, entry_level):

return pd_cls(raw[finite_mask], pd.MultiIndex.from_frame(index_df))

def _insert_aligned_columns(self, df, columns):
@staticmethod
def insert_aligned_columns(df, columns):
Copy link
Member

Choose a reason for hiding this comment

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

Is it a deliberate choice to make this method public? I think it will also appear in the docs, and I tend to think that keeping the documented API relatively small, so people can easily see the methods they want, is a good thing. But YMMV. 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It kind of is. I don't expect wide usage of it for now, but it seemed useful to not restrict aligning columns to the loading code if it's actually independent of it.

@takluyver
Copy link
Member

The implementation looks good to me.

It doesn't seem 100% ideal to be making APIs that look like load_some_data(with=some_other_data), as opposed to some form of load_some_data() + some_other_data. But I think that's hard to avoid given the constraints of what you're doing here, so go ahead.

@philsmt
Copy link
Collaborator Author

philsmt commented May 14, 2024

Thanks for review!

Indeed I still want a PulsePattern.label_data-esque API that can take data in as many different formats as possible and align it. But it seemed unlikely to get this right quickly, so I opted for a smaller integration in the DLD component itself. It should be easy to continue supporting this API on top of a more generic one later.

@takluyver
Copy link
Member

In that case, LGTM

@philsmt philsmt force-pushed the feat/dld-flexible-extra-columns branch from 92c6859 to 5455926 Compare May 15, 2024 16:15
@philsmt philsmt merged commit 5b09a74 into master May 15, 2024
6 checks passed
@philsmt philsmt deleted the feat/dld-flexible-extra-columns branch May 15, 2024 16:17
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

2 participants