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

More flexible gdp adapter #237

Merged
merged 11 commits into from
Aug 29, 2023

Conversation

milancurcic
Copy link
Member

Fixes #94 and #235.

@milancurcic milancurcic requested a review from selipot August 24, 2023 18:57
@milancurcic
Copy link
Member Author

CI on Ubuntu and macOS has been failing since yesterday due to segmentation fault in codecov (tests otherwise pass). Windows is OK. I haven't found anything about this from a cursory search. It could be transient or not.

@selipot unless you really want codecov to succeed, I think we can safely merge.

@philippemiron
Copy link
Contributor

What happened to all the tests?

@milancurcic
Copy link
Member Author

@philippemiron see #237 (comment). Codecov is crashing in CI. It started as yesterday as far as I know.

@philippemiron
Copy link
Contributor

@philippemiron see #237 (comment). Codecov is crashing in CI. It started as yesterday as far as I know.

I see.. and it's passing for Windows. 😕

@milancurcic
Copy link
Member Author

For once something works in Windows :)

@philippemiron
Copy link
Contributor

I feel a Microsoft bias in here.. haha

Copy link
Member

@selipot selipot left a comment

Choose a reason for hiding this comment

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

I have been able to test the new features such as tmp_path and the testing for existing files, all good
I noticed the following issues:

  • using the FTP source 2.00 the ragged array is filled typically with one drifter less than the requested n_random_id. That behavior does not exist for experimental so I think it's ok.
  • I cannot create a parquet file: it requires to use to_awkward from the ragged array instance and this fails because clouddrift.raggedarray.to_akward expects "count" instead of "rowsize"

clouddrift/adapters/gdp1h.py Outdated Show resolved Hide resolved
@selipot
Copy link
Member

selipot commented Aug 25, 2023

For some reason I can't get my environment to recognize my latest commit and test it.

  • I'd like to know if it resolves the issue of not being able to generate the parquet file via awkward
  • Should it be moved to another PR?

@milancurcic
Copy link
Member Author

I'd say just keep it in this PR for simplicity.

@milancurcic
Copy link
Member Author

I'd like to know if it resolves the issue of not being able to generate the parquet file via awkward

Hmm, works for me. What about it doesn't work for you? Is there an error message?

@milancurcic
Copy link
Member Author

I cannot create a parquet file: it requires to use to_awkward from the ragged array instance and this fails because clouddrift.raggedarray.to_akward expects "count" instead of "rowsize"

@selipot A-ha, I see this now. OK, I think your commit worked, because I can write a parquet file via awkward.

@selipot
Copy link
Member

selipot commented Aug 25, 2023

I'd like to know if it resolves the issue of not being able to generate the parquet file via awkward

Hmm, works for me. What about it doesn't work for you? Is there an error message?

Wrote earlier: For some reason I can't get my environment to recognize my latest commit and test it.

So I am not that skilled at VS code!

clouddrift/adapters/gdp1h.py Outdated Show resolved Hide resolved
clouddrift/adapters/gdp1h.py Outdated Show resolved Hide resolved
clouddrift/adapters/gdp1h.py Outdated Show resolved Hide resolved
clouddrift/adapters/gdp6h.py Outdated Show resolved Hide resolved
clouddrift/adapters/gdp6h.py Outdated Show resolved Hide resolved
@selipot
Copy link
Member

selipot commented Aug 25, 2023

The tests fail because of rowsize

@milancurcic milancurcic merged commit 02152c7 into Cloud-Drift:main Aug 29, 2023
@milancurcic milancurcic deleted the more-flexible-gdp-adapter branch August 29, 2023 02: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.

Allow setting a download directory for individual GDP files
3 participants