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

⭐ new transfer.py module #382

Merged
merged 41 commits into from
Sep 26, 2024
Merged

⭐ new transfer.py module #382

merged 41 commits into from
Sep 26, 2024

Conversation

selipot
Copy link
Member

@selipot selipot commented Mar 16, 2024

This is the start of a draft PR to implement the transfer module for the wind driven models of Lilly and Elipot 2021 and Elipot and Gille 2009.

@selipot selipot added the enhancement New feature or request label Mar 16, 2024
@selipot selipot self-assigned this Mar 16, 2024
@selipot selipot marked this pull request as ready for review March 19, 2024 18:39
@selipot selipot marked this pull request as draft March 19, 2024 18:40
@selipot
Copy link
Member Author

selipot commented Mar 19, 2024

More tests are probably needed but it is in good shape.

@philippemiron philippemiron marked this pull request as ready for review March 20, 2024 02:37
clouddrift/__init__.py Show resolved Hide resolved
clouddrift/transfer.py Outdated Show resolved Hide resolved
clouddrift/transfer.py Show resolved Hide resolved
clouddrift/transfer.py Outdated Show resolved Hide resolved
clouddrift/transfer.py Outdated Show resolved Hide resolved
clouddrift/transfer.py Show resolved Hide resolved
clouddrift/transfer.py Outdated Show resolved Hide resolved
clouddrift/transfer.py Show resolved Hide resolved
clouddrift/transfer.py Show resolved Hide resolved
tests/transfer_tests.py Outdated Show resolved Hide resolved
@selipot
Copy link
Member Author

selipot commented Mar 21, 2024

For this PR I want to use the type declaration/typing using python 3.10 conventions ( PEP 604 and PEP 585). @kevinsantana11 should we have another PR before that to remove the test suite for Python 3.9?

I agree that PEP 604, in particular, is much nicer and simplifies the syntax a lot, but I would suggest we merge it and then do a single PR to fix all the types.

It looks like you already made the modification to support 3.9, correct?

Right now I believe the way I wrote the code is being supported by 3.9. So yes we can do a later PR. Hopefully with some shell scripting tool like sed to automatically replace all the typing.

@philippemiron
Copy link
Contributor

@kevinsantana11 kevinsantana11 changed the title new transfer.py module ⭐ new transfer.py module Mar 23, 2024
@philippemiron
Copy link
Contributor

Is it normal that it doesn't pass the test now?

@selipot
Copy link
Member Author

selipot commented Apr 1, 2024

Is it normal that it doesn't pass the test now?

Yes, working on this. It is a draft and there are still some problems.

@philippemiron philippemiron marked this pull request as draft April 1, 2024 19:02
@philippemiron philippemiron marked this pull request as ready for review April 2, 2024 13:37
selipot and others added 4 commits April 2, 2024 09:45
* Add docs focused on contributing and document several processes

-----

Co-authored-by: Philippe Miron <[email protected]>
Co-authored-by: Shane Elipot <[email protected]>
* hurdat2 data adapter

* use CF conventions

* improve warning when it cannot be determined whether the remote file is new or not

* increment revision

---------

Co-authored-by: Shane Elipot <[email protected]>
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 77.24138% with 66 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
clouddrift/transfer.py 77.16% 66 Missing ⚠️
Additional details and impacted files

📢 Thoughts on this report? Let us know!

@selipot
Copy link
Member Author

selipot commented Sep 11, 2024

@kevinsantana11 I think we should merge, what do you think?

@selipot
Copy link
Member Author

selipot commented Sep 11, 2024

I'll start a demo repo notebook next

@selipot
Copy link
Member Author

selipot commented Sep 26, 2024

@kevinsantana11 can we merge?

@kevinsantana11
Copy link
Contributor

@selipot yes, please go ahead and merge

@selipot selipot merged commit 2327faf into Cloud-Drift:main Sep 26, 2024
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants