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

Refactor utility module #490

Merged
merged 12 commits into from
Aug 23, 2021
Merged

Conversation

vhirtham
Copy link
Collaborator

@vhirtham vhirtham commented Aug 20, 2021

Changes

Separates xarray related utility functions from other utility functions and removes no longer needed functions.

Related Issues

Closes #480

Checks

  • updated CHANGELOG.md
  • updated tests
  • updated doc/
  • update example/tutorial notebooks

@vhirtham vhirtham self-assigned this Aug 20, 2021
@pep8speaks
Copy link

pep8speaks commented Aug 20, 2021

Hello @vhirtham! Thanks for updating this PR.

Line 1:5: E999 SyntaxError: invalid syntax

Comment last updated at 2021-08-23 07:31:33 UTC

@codecov
Copy link

codecov bot commented Aug 20, 2021

Codecov Report

Merging #490 (498631d) into master (fe0f622) will decrease coverage by 0.02%.
The diff coverage is 98.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #490      +/-   ##
==========================================
- Coverage   96.86%   96.84%   -0.03%     
==========================================
  Files          89       91       +2     
  Lines        5547     5543       -4     
==========================================
- Hits         5373     5368       -5     
- Misses        174      175       +1     
Impacted Files Coverage Δ
weldx/asdf/file.py 97.43% <ø> (ø)
weldx/asdf/util.py 84.88% <ø> (ø)
weldx/geometry.py 99.44% <95.65%> (-0.13%) ⬇️
weldx/util/util.py 99.11% <99.11%> (ø)
weldx/util/__init__.py 100.00% <100.00%> (ø)
weldx/util/xarray.py 99.10% <100.00%> (ø)

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 fe0f622...498631d. Read the comment docs.

@vhirtham vhirtham marked this pull request as ready for review August 23, 2021 07:53
@vhirtham
Copy link
Collaborator Author

Looking at the remaining functions in the util module, I think it might make sense to mark it as private. Apart from some xarray functions, most of the stuff is not of any interest to the user. Even the xarray functions probably shouldn't be used outside of the package. What do you think @CagtayFabry @marscher

In case we want to keep it public, we need to discuss how the doc pages should look like. Currently, you get to an intermediate site that lists both submodules. I think this is okay this way to further categorize the functions and classes.

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.

Looking at the remaining functions in the util module, I think it might make sense to mark it as private. Apart from some xarray functions, most of the stuff is not of any interest to the user. Even the xarray functions probably shouldn't be used outside of the package. What do you think @CagtayFabry @marscher

In case we want to keep it public, we need to discuss how the doc pages should look like. Currently, you get to an intermediate site that lists both submodules. I think this is okay this way to further categorize the functions and classes.

I actually like it the way it is now. 👍
Since the utility functions are in their own module now and don't clutter anything I like having them in the documentation :)
Especially for the xarray functions since they probably become much more handy when working more with spatial data or the new geometry stuff later. (we should probably flesh out the docstrings some more though)

@vhirtham
Copy link
Collaborator Author

Looking at the remaining functions in the util module, I think it might make sense to mark it as private. Apart from some xarray functions, most of the stuff is not of any interest to the user. Even the xarray functions probably shouldn't be used outside of the package. What do you think @CagtayFabry @marscher
In case we want to keep it public, we need to discuss how the doc pages should look like. Currently, you get to an intermediate site that lists both submodules. I think this is okay this way to further categorize the functions and classes.

I actually like it the way it is now. 👍
Since the utility functions are in their own module now and don't clutter anything I like having them in the documentation :)
Especially for the xarray functions since they probably become much more handy when working more with spatial data or the new geometry stuff later. (we should probably flesh out the docstrings some more though)

I think that all the functionality they provide should be wrapped by SpatialData and the corresponding geometry functions and classes. But we can leave them public for now and reevaluate later.

@vhirtham vhirtham merged commit cd7f2f3 into BAMWelDX:master Aug 23, 2021
@vhirtham vhirtham deleted the 480_refactor_util branch August 23, 2021 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up utility package
3 participants