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

Remove redundant coords assignment; test for dims and coords #324

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

milancurcic
Copy link
Member

@milancurcic milancurcic commented Nov 14, 2023

I don't know why this fixes this index dim on some systems, but it seems like it does. I added tests to ensure that only obs and traj are in dims and coords.

@selipot @philippemiron please check on your ends.

Closes #323.

@milancurcic milancurcic self-assigned this Nov 14, 2023
@milancurcic milancurcic added the bug Something isn't working label Nov 14, 2023
@selipot
Copy link
Member

selipot commented Nov 14, 2023

Ok index is gone.

But why not having like for the GDP the coordinates time(obs) and ids(traj) instead of obs(obs) and traj(traj)? I find this not really comprehensible. The coordinate you have currently called obs is effectively a time variable and the traj is a unique identification string.

Screenshot 2023-11-14 at 6 54 42 PM

@philippemiron
Copy link
Contributor

Works for me too. Only comment: should we put a message to notify the user when the file already exists in ~/.clouddrift/data/?

@milancurcic
Copy link
Member Author

Yes, let's discuss the naming convention.

My preference may be time(time) and id(id). Unclear to me why name coords differently from dims, as they map 1-to-1 in our case.

@philippemiron
Copy link
Contributor

And I agree with Shane. I'm not sure I understand the addition of obs and traj as coordinates either.

@milancurcic
Copy link
Member Author

As discussed on the call, we will rename obs(obs) and traj(traj) coordinates to time(obs) and id(traj) respectively. Dimension names remain the same.

@milancurcic milancurcic merged commit 7d4514e into Cloud-Drift:main Nov 15, 2023
@milancurcic milancurcic deleted the glad-fix branch November 15, 2023 17:18
philippemiron pushed a commit to philippemiron/clouddrift that referenced this pull request Nov 16, 2023
…rift#324)

* Remove redundant coords assignment; test for dims and coords

* Rename GLAD coordinates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GLAD dataset structure?
3 participants