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

Append coordinate reference frame to dataframe column names #40

Closed
wants to merge 1 commit into from

Conversation

moeyensj
Copy link
Member

@moeyensj moeyensj commented Aug 15, 2023

This requires #26 to be merged to main.

@moeyensj moeyensj mentioned this pull request Aug 15, 2023
Copy link
Contributor

@spenczar spenczar left a comment

Choose a reason for hiding this comment

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

The tests look good, and the code looks correct, but... yeesh, this is unpleasant stuff.

I suppose this works, but we must keep in mind that this won't get called if the coordinates are used as a subtable, like in Observers or whatever.

Perhaps quivr.to_dataframe and quivr.from_dataframe should support serializing attributes as a (repeated) column in dataframes, including for sub-tables, as part of the flattening process. Something like coords.to_dataframe(include_attrs=True).

But before doing that, I guess I should ask: what are the scenarios where you are using pandas dataframes? That might help inform the designs.

@moeyensj
Copy link
Member Author

But before doing that, I guess I should ask: what are the scenarios where you are using pandas dataframes? That might help inform the designs.

Mainly for a more eye-friendly inspection of the underlying tables.

I'm going to close this because it's a bit.. grotesque to encode attributes as column names. I think we should just be diligent to not serialize to/from dataframes in a production setting and instead save things with quivr built-in functionality.

@moeyensj moeyensj closed this Aug 24, 2023
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