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 weldx file wrapper #341

Merged
merged 70 commits into from
May 4, 2021
Merged

Conversation

marscher
Copy link
Collaborator

@marscher marscher commented Apr 16, 2021

Changes

This adds an ASDF file wrapper to ease some

Features

  • Dictionary interface.
  • Provides an optional interface to access the dictionary via attributes.
  • Handle file objects internally, e.g. create buffers automatically.
  • Context manager interface, synchronizing file contents.
  • Expose only the most mandatory ASDF interfaces to users.
  • Easy visualization of the Asdf header.
  • History entries with custom software entries.

Related Issues

Closes #271

Checks

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

@marscher marscher added the ASDF everything ASDF related (python + schemas) label Apr 16, 2021
@pep8speaks
Copy link

pep8speaks commented Apr 16, 2021

Hello @marscher! Thanks for updating this PR.

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

Comment last updated at 2021-05-04 13:18:20 UTC

weldx/asdf/file.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 20, 2021

Codecov Report

Merging #341 (dbe3b62) into master (19b2e97) will decrease coverage by 0.05%.
The diff coverage is 94.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #341      +/-   ##
==========================================
- Coverage   96.95%   96.89%   -0.06%     
==========================================
  Files          85       87       +2     
  Lines        4993     5192     +199     
==========================================
+ Hits         4841     5031     +190     
- Misses        152      161       +9     
Impacted Files Coverage Δ
weldx/util.py 98.96% <77.77%> (-0.51%) ⬇️
weldx/asdf/util.py 80.24% <85.00%> (+0.79%) ⬆️
weldx/asdf/file.py 96.25% <96.25%> (ø)
weldx/__init__.py 100.00% <100.00%> (ø)
weldx/types.py 100.00% <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 19b2e97...dbe3b62. Read the comment docs.

weldx/asdf/file.py Outdated Show resolved Hide resolved
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@marscher
Copy link
Collaborator Author

cd'ing out of test source directory helps to solve this import conflict thing pytest raised, but now we have to decide how we proceed with the missing "scripts" folder. Currently the single_pass_weld.py is used as a fixture in several tests. When this is not deployed/installed, we should decide where to move these scripts, so that they can be used in an installed version of weldx.
I'd suggest to have entry points for these scripts (so they can be used from the command line) and move the contents somewhere within the core package.

@CagtayFabry, @vhirtham what do you think where we should keep these scripts?

@CagtayFabry
Copy link
Member

cd'ing out of test source directory helps to solve this import conflict thing pytest raised, but now we have to decide how we proceed with the missing "scripts" folder. Currently the single_pass_weld.py is used as a fixture in several tests. When this is not deployed/installed, we should decide where to move these scripts, so that they can be used in an installed version of weldx.
I'd suggest to have entry points for these scripts (so they can be used from the command line) and move the contents somewhere within the core package.

@CagtayFabry, @vhirtham what do you think where we should keep these scripts?

If this is solved by #350 that's fine by me

@@ -1335,3 +1336,17 @@ def compare_nested(


compare_nested = _Eq_compare_nested.compare_nested


def is_interactive_session() -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

I would be really interested to see if/how we can distinguish between jupyter notebook/lab here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't seem to be possible as both environments seem to be equal from within the ipython object.
There has been a request to enable the display of JSON, but there was no response since last year. So I guess this has no priority at all. jupyter/notebook#5446

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.

I didn't get to review all code segments in detail but I think we should start using this as soon as possible to get a feel for it :)

weldx/asdf/file.py Outdated Show resolved Hide resolved
weldx/asdf/file.py Outdated Show resolved Hide resolved
weldx/asdf/file.py Outdated Show resolved Hide resolved
weldx/asdf/file.py Outdated Show resolved Hide resolved
weldx/asdf/file.py Outdated Show resolved Hide resolved
weldx/asdf/file.py Outdated Show resolved Hide resolved
@marscher
Copy link
Collaborator Author

marscher commented May 4, 2021 via email

@vhirtham
Copy link
Collaborator

vhirtham commented May 4, 2021

A setter will never be rendered in sphinx, probably that's why.

You are right: https://stackoverflow.com/questions/17478040/how-can-i-generate-documentation-for-a-python-property-setter-using-sphinx

Good to know.

@marscher
Copy link
Collaborator Author

marscher commented May 4, 2021

Unfortunately there is no way to determine, if a code is running in jupyterlab or notebook. So I suggest, we fall back to set use_widgets to false and lab users can enable it on their behalf.

@marscher marscher merged commit dd32b8a into BAMWelDX:master May 4, 2021
@marscher marscher deleted the add_weldx_file_wrapper branch May 4, 2021 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASDF everything ASDF related (python + schemas) core weldx core classes and functions KISA issues related to the KISA project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

asdffile wrapper discussion
4 participants