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

Fix accumulate_metadata replacing coordinates with Nones #88

Merged
merged 4 commits into from
Nov 15, 2021

Conversation

gjoseph92
Copy link
Owner

Fixes #78

While reviewing #87, I realized there were many things wrong with accumulate_metadata. The function just had too many cases and was trying to do too many things. #40 allowed for some incorrect code paths which we aren't actually using, but would be annoying to handle correctly. So instead, I basically redid #40, splitting accumulate_metadata into one function for the normal properties case, and one for the asset-metadata case where we only care about fields that have the same value everywhere.

@scottyhq I believe this fixes your example, but would be good to confirm:
Screen Shot 2021-11-14 at 1 17 15 AM

This function was trying to support too many cases; its behavior was actually incorrect for a lot of them. Simplified by splitting into two different functions for the two main cases we care about.
@scottyhq
Copy link
Contributor

@scottyhq I believe this fixes your example, but would be good to confirm:

Looks good to me, thanks for working on it! Out of curiosity how are you getting the times output in each cell in the screenshot you included? Feel free to close #87 in favor of this.

@gjoseph92
Copy link
Owner Author

how are you getting the times output in each cell in the screenshot you included?

Ah, this is just using the jupyter notebook feature in VSCode: https://code.visualstudio.com/docs/datascience/jupyter-notebooks. It's gotten a lot better in the last year, you can pretty much use it for anything that doesn't require widgets (and then you get the much better editor and autocomplete).

@gjoseph92 gjoseph92 merged commit 3a58906 into main Nov 15, 2021
@gjoseph92 gjoseph92 deleted the fix-accumulate-metadata branch November 15, 2021 19:32
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.

Puzzling behavior of accumulate_metadata (None and NaTs in coordinates)
2 participants