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

Python translation of fcDatetick (plus some extra testing for datenum and a fix) #103

Merged
merged 24 commits into from
Jan 27, 2025

Conversation

dorchard
Copy link
Member

@dorchard dorchard commented Jan 9, 2025

Note this is mostly a debugging plotting routine and its tests are largely a smoke test over various options so it's relatively low priority in terms of its exact matching MATLAB. After discussion with Gilberto we decided we don't really need it.

  • Minimal translation of the main cases
  • Associated smoke tests (that all pass)
  • Documentation (including an explanation that it's not used in the rest of the code and why)

Along the way I discovered though that our implementation of datenum needs improving and so this PR also adds separate tests for this function and some updates

  • Fixes to datenum
  • Separate testing routine for datenum

@dorchard dorchard changed the title Python translation of fcDatetick Python translation of fcDatetick (plus some extra testing for datenum and a fix) Jan 15, 2025
@dorchard dorchard marked this pull request as ready for review January 15, 2025 17:06
@dorchard dorchard requested a review from isaacaka January 15, 2025 17:06
@isaacaka
Copy link
Member

All tests pass, I pushed a fix to the overwritten site data and removed a typo which caused the test to fail. I think it's good to merge

Copy link
Member

@j-emberton j-emberton left a comment

Choose a reason for hiding this comment

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

Looks good. All matlab and python tests for this PR pass

@@ -1,6 +1,6 @@
import numpy
from fcDatevec import mydatevec
from fcDatenum import mydatenum
from fcDatenum import datenum

def mydoy(t):
Copy link
Member

Choose a reason for hiding this comment

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

typehints

oneflux_steps/ustar_cp_python/fcDatenum.py Outdated Show resolved Hide resolved
oneflux_steps/ustar_cp_python/fcDatetick.py Outdated Show resolved Hide resolved
oneflux_steps/ustar_cp_python/fcDoy.py Show resolved Hide resolved
@dorchard
Copy link
Member Author

Added the type hints now and also made a change to the test to remove a latent use of explicit matlab engine @j-emberton

@dorchard dorchard requested a review from j-emberton January 27, 2025 14:52
@dorchard dorchard merged commit d3818e6 into ustar_cp_refactor_main Jan 27, 2025
@dorchard dorchard deleted the python-fcDatetick branch January 29, 2025 12:04
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.

3 participants