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 dataset tests #14

Merged
merged 23 commits into from
Sep 19, 2023
Merged

Add dataset tests #14

merged 23 commits into from
Sep 19, 2023

Conversation

sphamba
Copy link
Collaborator

@sphamba sphamba commented Sep 13, 2023

Prework

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Documentation
  • Tutorial
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and communicate accordingly:

The PR fulfills these requirements:

  • It's submitted to the branch named as follow :
    • Fix a bug: bugfix-<some_key>-<word>
    • Improve the doc: doc-<some_key>-<word>
    • Improve a tutorial tutorial-<some_key>-<word>
    • Add a new feature: feature-<some_key>-<word>
    • Refactor some code: refactor-<some_key>-<word>
    • Optimize some code: optimize-<some_key>-<word>
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • Don't forget to link PR to issue if you are solving one.
  • All tests are passing.
  • New/updated tests are included

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Summary

  • Fix coverage in CI
  • Add tests for gpm_api/dataset/

evanjt and others added 22 commits August 18, 2023 03:24
…lter_by_time(), modify the default None behaviour to choose utcnow() instead of now() to avoid timezone related issues
@coveralls
Copy link

coveralls commented Sep 13, 2023

Coverage Status

coverage: 41.632% (+41.6%) from 0.0% when pulling ac2d83b on EPFL-ENAC:add_dataset_tests into e39dc20 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.

LGTM. We could already merge this if you want

"""Set dataset attributes for each attrs_dict key."""
for var in attrs_dict:
ds[var].attrs.update(attrs_dict[var])

Copy link
Owner

Choose a reason for hiding this comment

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

Not necessarily to be addressed in this PR, but let's try to update the code to return the ds in such type of functions ;)
So that ds = set_whatever(ds)

dataarray = xr.DataArray(array, dims=["used_dim"])
dataset = xr.Dataset(data_vars={"var": dataarray})
dataset = dataset.expand_dims(dim=["unused_dim"])
# TODO: this does not work, it adds the dimension to all variables. How can this be done?
Copy link
Owner

Choose a reason for hiding this comment

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

I see. I guess to create the test case, you need to create two data arrays, then to one of them you call the expand_dims, then you xr.merge, and then you drop the variable with the "unused dim".
It might be that you need to define the dimension coordinate to avoid that by dropping the variable it drops also the dimension (because completely unused)

@sphamba sphamba requested a review from evanjt September 15, 2023 15:11
@sphamba
Copy link
Collaborator Author

sphamba commented Sep 15, 2023

I'll address your comment on test_granule and then we can indeed merge to ease your next review :)

@ghiggi
Copy link
Owner

ghiggi commented Sep 19, 2023

@sphamba can I merge now?

@sphamba
Copy link
Collaborator Author

sphamba commented Sep 19, 2023

@sphamba can I merge now?

Yes!

@ghiggi ghiggi marked this pull request as ready for review September 19, 2023 12:44
@ghiggi ghiggi merged commit 2fc2e2b into ghiggi:main Sep 19, 2023
9 of 10 checks passed
@ghiggi ghiggi deleted the add_dataset_tests branch September 19, 2023 12:45
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.

4 participants