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

Add utils tests #24

Merged
merged 18 commits into from
Dec 7, 2023
Merged

Add utils tests #24

merged 18 commits into from
Dec 7, 2023

Conversation

sphamba
Copy link
Collaborator

@sphamba sphamba commented Dec 4, 2023

Summary

Add tests for:

  • utils/checks.py
  • utils/slices.py
  • utils/time.py

@sphamba sphamba requested a review from ghiggi December 4, 2023 16:07
@coveralls
Copy link

coveralls commented Dec 4, 2023

Coverage Status

coverage: 52.743% (+8.8%) from 43.96%
when pulling 41baf42 on EPFL-ENAC:feat-utils-tests
into 11c8b3f on ghiggi:main.

Copy link
Owner

@ghiggi ghiggi left a comment

Choose a reason for hiding this comment

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

Thanks for all the work you put into this !
I added just a couple of points to address before merging the PR.

If you want, you can already change the behaviour of all get_slices_* to return a list of dictionary {<dim>: <slice>} (or IselDict class (see below) instead of <slice> objects. <dim> will be along_track or time depending on the gpm object type ;) This will allow to directly pass each element of the list to the xarray isel method ... without the need to specify the dimension.

I see two possible implementations:

  • One solution is to make all current get_slices functions private and then define a new get_slices_* functions that call the corresponding private functions and transform the list of slices in list of dictionaries.
  • Another solution is to add an argument like return_isel_dict=True to all current get_slices_* functions.

We could return an IselDicts class

  • which is initiated with a list of {<dim>: <slice>}
  • with an __iter__ method so that you can easily loop over the list element or index by position
  • with the union/intersects/difference methods that allows the user to combine various isel_dicts objects derived from the various get_slices_* function. These methods should check that all are equal, and otherwise raise an error ;)

We can discuss about this in the coming days, and if you prefer I can also take care of implementing it ;)

Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (dd1d1fc) 43.95% compared to head (41baf42) 52.74%.
Report is 1 commits behind head on main.

Files Patch % Lines
gpm_api/utils/checks.py 95.23% 1 Missing ⚠️
gpm_api/utils/slices.py 97.36% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #24      +/-   ##
==========================================
+ Coverage   43.95%   52.74%   +8.78%     
==========================================
  Files          64       64              
  Lines        5339     5377      +38     
==========================================
+ Hits         2347     2836     +489     
+ Misses       2992     2541     -451     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sphamba
Copy link
Collaborator Author

sphamba commented Dec 7, 2023

I adressed your comments! Regarding the change of behavior of get_slices_*, we can do it in a separate PR, and we can see where you want me to spend time

Copy link
Owner

@ghiggi ghiggi left a comment

Choose a reason for hiding this comment

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

LGTM

@ghiggi ghiggi merged commit 2d72760 into ghiggi:main Dec 7, 2023
@sphamba sphamba deleted the feat-utils-tests branch March 13, 2024 08:20
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