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

23 xarray utility #125

Merged
merged 49 commits into from
Oct 9, 2020
Merged

23 xarray utility #125

merged 49 commits into from
Oct 9, 2020

Conversation

mark141
Copy link
Contributor

@mark141 mark141 commented Sep 8, 2020

The following checks are now implemented:

  • existence of certain dimensions
  • allowed coordinate dtypes
  • specific coordinate values

Input should be a nested dict with an array for the values:

ref = {
     "time" : {
        "optional" : True,
        "dtype" : ["datetime64[ns]","timedelta64[ns]"] },
    "c" : {"values" : ["x","y","z"]}
}

usage:
utility.xr_check_coords(dax: xr.DataArray, ref: dict)
utility.xr_check_dtype(dax: xr.DataArray, ref: dict)

missing:

  • Do we want to raise Errors when the check fails or return a Value? Or maybe just a Warning?
  • @reviewers do you confirm the change of the values to an array?
  • Docstrings (depending on the missing solutions)

@mark141 mark141 added the xarray issues related to handling xarray objects label Sep 8, 2020
@mark141 mark141 requested a review from CagtayFabry September 8, 2020 11:14
@mark141 mark141 self-assigned this Sep 8, 2020
@codecov
Copy link

codecov bot commented Sep 8, 2020

Codecov Report

Merging #125 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #125   +/-   ##
=======================================
  Coverage   99.53%   99.54%           
=======================================
  Files         126      126           
  Lines        6952     7016   +64     
=======================================
+ Hits         6920     6984   +64     
  Misses         32       32           
Impacted Files Coverage Δ
weldx/transformations.py 98.01% <100.00%> (+<0.01%) ⬆️
weldx/utility.py 98.11% <100.00%> (+0.31%) ⬆️
D:/a/weldx/weldx/weldx/transformations.py 98.01% <0.00%> (+<0.01%) ⬆️
D:/a/weldx/weldx/weldx/utility.py 98.11% <0.00%> (+0.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a545f07...781b4fe. Read the comment docs.

Copy link
Member

@CagtayFabry CagtayFabry 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 👍

  • maybe add more test cases
  • add docstrings (ideally with examples)
  • implement missing functions
  • code and repo cleanup

23_test.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@vhirtham vhirtham left a comment

Choose a reason for hiding this comment

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

Added some reminders via review so that those issues won't be forgotten

tests/test_utility.py Outdated Show resolved Hide resolved
tests/test_utility.py Outdated Show resolved Hide resolved
tests/test_utility.py Outdated Show resolved Hide resolved
weldx/utility.py Outdated Show resolved Hide resolved
weldx/utility.py Outdated Show resolved Hide resolved
weldx/utility.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Sep 28, 2020

Hello @mark141! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-10-09 08:13:03 UTC

@mark141
Copy link
Contributor Author

mark141 commented Sep 28, 2020

Added functionalities are now all in one function xr_check_coords(dax: xr.DataArray, ref: dict).
Functionalities:

  • "optional" keyword: if the DataArray does not have this dimension
  • "values" keyword: comparing the values of the the DataArray with the ones in the reference dictionary
  • "dtype" keyword: comparing the numpy dtypes. Additional all numpy strings are now comparable with str
  • Output is True or the corresponding Exception.

Other achievements:

  • Pytests now test every line of the code.
  • All Tests and functions got docstrings! And only one language is used in the whole code!
  • There is even a small example in the docstring of xr_check_coords.

@mark141 mark141 requested a review from CagtayFabry October 5, 2020 12:15
Copy link
Member

@CagtayFabry CagtayFabry left a comment

Choose a reason for hiding this comment

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

  • add support for general "timedelta64" and "datetime64" dtype description as discussed
  • add changelog entry

@mark141 mark141 requested a review from CagtayFabry October 7, 2020 10:16
tests/test_utility.py Outdated Show resolved Hide resolved
tests/test_utility.py Outdated Show resolved Hide resolved
tests/test_utility.py Outdated Show resolved Hide resolved
tests/test_utility.py Outdated Show resolved Hide resolved
weldx/utility.py Outdated Show resolved Hide resolved
weldx/utility.py Outdated Show resolved Hide resolved
@mark141 mark141 requested a review from CagtayFabry October 7, 2020 14:05
weldx/utility.py Outdated Show resolved Hide resolved
Copy link
Member

@CagtayFabry CagtayFabry left a comment

Choose a reason for hiding this comment

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

see my comment for final code changes :)

mark141 and others added 3 commits October 7, 2020 16:24
suggestion accepted!

Co-authored-by: Cagtay Fabry <43667554+CagtayFabry@users.noreply.github.com>
@CagtayFabry CagtayFabry requested a review from vhirtham October 7, 2020 14:41
Copy link
Collaborator

@vhirtham vhirtham 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 so far. My comments are just nitpicking, but the stuff should be added relatively easy

weldx/utility.py Outdated Show resolved Hide resolved
weldx/utility.py Outdated Show resolved Hide resolved
weldx/utility.py Outdated Show resolved Hide resolved
weldx/utility.py Outdated Show resolved Hide resolved
@CagtayFabry CagtayFabry merged commit ca9cf39 into master Oct 9, 2020
@CagtayFabry CagtayFabry deleted the 23_xarray_utility branch October 9, 2020 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
xarray issues related to handling xarray objects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add utility function for checking dimensions/coordinates of xarray objects
4 participants