Fix failing tests and documentation #684

vhirtham · 2022-01-25T13:55:11Z

Changes

Fix failing tests due to new pandas version ->seems like pandas added own __add__ functions so that we couldn't use + with a pandas Type on the left-hand side and our Time class on the right-hand side
Fix failing documentation due to a cross reference -> weldx.time.Time vs weldx.Time

Related Issues

None

Checks

codecov · 2022-01-25T14:04:34Z

Codecov Report

Merging #684 (823609f) into master (fe21561) will increase coverage by 0.00%.
The diff coverage is 93.75%.

@@           Coverage Diff           @@
##           master     #684   +/-   ##
=======================================
  Coverage   96.07%   96.08%           
=======================================
  Files          92       92           
  Lines        6325     6327    +2     
=======================================
+ Hits         6077     6079    +2     
  Misses        248      248

Impacted Files	Coverage Δ
weldx/transformations/cs_manager.py	`98.79% <ø> (ø)`
weldx/asdf/util.py	`90.20% <50.00%> (ø)`
weldx/asdf/cli/welding_schema.py	`97.02% <100.00%> (+0.02%)`	⬆️
weldx/asdf/file.py	`97.04% <100.00%> (ø)`
weldx/tags/debug/test_shape_validator.py	`100.00% <100.00%> (ø)`
weldx/tags/debug/test_unit_validator.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 fe21561...823609f. Read the comment docs.

github-actions · 2022-01-25T14:06:59Z

Unit Test Results

      1 files ±0       1 suites ±0 2m 7s ⏱️ -3s
2 101 tests ±0 2 101 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 823609f. ± Comparison against base commit fe21561.

♻️ This comment has been updated with latest results.

review-notebook-app · 2022-01-25T16:45:11Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

vhirtham · 2022-01-25T17:27:30Z

Finally, I got the documentation bug fixed... sort of (see here). During the process I went nuts and adjusted all imports to import from the correct module instead of the global weldx namespace. Maybe we have to review where we want to undo that (doc examples, tutorials?). On the plus side, it should probably also fix some circular imports that poped up every now and then.

marscher · 2022-01-25T19:16:32Z

Thanks for fixing this pandas issue.

Was it really mandatory to change all the imports? What was the exact error message?

vhirtham · 2022-01-26T08:00:44Z

Thanks for fixing this pandas issue.

Was it really mandatory to change all the imports? What was the exact error message?

The fix for the pandas issue was the first commit and I just had to adjust the tests to the new behaviour.

The fix for the documantation was the linked commit in my previous comment. So the import changes weren't mandatory. However, I couldn't reproduce the bug locally so I had to try it by committing over and over again. I initially thaught sphinx was getting the weldx.Time definition from a direct weldx import. So I changed them to import from the correct module. After I changed all I could find, the error was still not fixed, so I removed the Type hint entirely and that did the trick.

As I said, we can undo all the import changes, but I think at least some of them do make sense since I get circular import problems from time to time when I run the debugger. But in the notebooks and examples, I feel we should undo it since it bloats the code and gives the wrong impression that you always have to pick the correct module. Feel free to mark things to be undone. Maybe we can discuss this during todays jour fixe.

This was the sphinx error message:

/home/runner/work/weldx/weldx/weldx/geometry.py:docstring of weldx.geometry.SpatialData.time:: WARNING: more than one target found for cross-reference 'Time': weldx.time.Time, weldx.Time

CagtayFabry · 2022-01-26T08:36:01Z

weldx/core.py

+        >>> from weldx.core import GenericSeries
+        >>> from weldx.constants import Q_


is there a bug/import error when we import from the main weldx namespace in the doctests?

See my comment above --- no there isn't.

CagtayFabry · 2022-01-26T08:45:54Z

Ok I just now caught up on the comments

I think in the python package files it is better/safer to import from the correct namespace to avoid potential import errors.
In the doctest examples that should not be needed and makes it less readable, so my vote goes to revert to top level imports here

Regarding the pandas issues:
From what I see we now skip the pandas types in the tests, but what is the actual behavior/error that comes up?
Can you maybe link to a CI run that failed with the new pandas version @vhirtham ?

CagtayFabry · 2022-01-26T08:48:38Z

regarding the sphinx error message WARNING: more than one target found for cross-reference 'Time': weldx.time.Time, weldx.Time

I think this is mostly due to the fact that we have both elements weldx.Time and weldx.time.Time in our documentation toctree after #437 ? So nothing related to the python package per se?

marscher · 2022-01-26T09:56:01Z

regarding the sphinx error message WARNING: more than one target found for cross-reference 'Time': weldx.time.Time, weldx.Time

I think this is mostly due to the fact that we have both elements weldx.Time and weldx.time.Time in our documentation toctree after #437 ? So nothing related to the python package per se?

The same thing applies to CSM and LCS for instance, since they live in their own modules and are pulled to the top-level namespace. So there was a trick, how to avoid this ambiguity, but I cannot wrap my head around it right now.

vhirtham · 2022-01-26T10:41:51Z

Regarding the pandas issues: From what I see we now skip the pandas types in the tests, but what is the actual behavior/error that comes up? Can you maybe link to a CI run that failed with the new pandas version @vhirtham ?

Here you go: https://github.com/BAMWelDX/weldx/runs/4936771641?check_suite_focus=true

CagtayFabry · 2022-01-26T12:23:02Z

Regarding the pandas issues: From what I see we now skip the pandas types in the tests, but what is the actual behavior/error that comes up? Can you maybe link to a CI run that failed with the new pandas version @vhirtham ?

Here you go: https://github.com/BAMWelDX/weldx/runs/4936771641?check_suite_focus=true

Interesting, it looks like TimedeltaIndex + Time now returns a TimedeltaIndex even though I don't know how they do it... (I guess because we implemented __iter__ or similar so Time is accessed as an array)

CagtayFabry · 2022-01-26T17:59:51Z

I have merged the changes to fix the errors in the other PRs #689 and #678 @vhirtham
We can merge the internal import cleanups here

tutorials/01_02_time_dependent_data.ipynb

weldx/constants.py

weldx/core.py

weldx/measurement.py

weldx/time.py

weldx/transformations/cs_manager.py

weldx/transformations/local_cs.py

vhirtham · 2022-01-27T13:53:44Z

@CagtayFabry All the doc changes are undone and some additional corrections applied. If you find the time to revisit, that would be nice. Otherwise, I ll merge tomorrow

Fix pandas 1.4.0 related issue

ebd6c26

vhirtham added bug Something isn't working documentation Improvements or additions to documentation tests pytest (and other tests) related labels Jan 25, 2022

vhirtham added 5 commits January 25, 2022 16:06

Try doc fix

2866d1c

Try doc fix

8f0c5a5

Try doc fix

df57931

Try doc fix

fe24ee8

Try doc fix

928c735

vhirtham added 2 commits January 25, 2022 17:57

Try doc fix

2b2bcf8

Fix doc

11a63cb

Update changelog

849b698

vhirtham requested review from CagtayFabry and marscher January 25, 2022 17:28

Fix changelog ref

df6b3c1

vhirtham mentioned this pull request Jan 26, 2022

run PEP585 upgrade #678

Merged

CagtayFabry reviewed Jan 26, 2022

View reviewed changes

Merge branch 'master' into fix_latest_issues

5045df9

marscher approved these changes Jan 27, 2022

View reviewed changes

Merge branch 'master' into fix_latest_issues

cdae6b6

vhirtham commented Jan 27, 2022

View reviewed changes

vhirtham added 4 commits January 27, 2022 11:34

Apply suggestions from code review

bc12369

Fix mypy issue

d2cac2c

Update docstrings

caaf42e

Fix doctest

79010c5

Merge branch 'master' into fix_latest_issues

823609f

vhirtham merged commit 2cc7b20 into BAMWelDX:master Jan 28, 2022

vhirtham deleted the fix_latest_issues branch January 28, 2022 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix failing tests and documentation #684

Fix failing tests and documentation #684

vhirtham commented Jan 25, 2022 •

edited

Loading

codecov bot commented Jan 25, 2022 •

edited

Loading

github-actions bot commented Jan 25, 2022 •

edited

Loading

review-notebook-app bot commented Jan 25, 2022

vhirtham commented Jan 25, 2022

marscher commented Jan 25, 2022

vhirtham commented Jan 26, 2022 •

edited

Loading

CagtayFabry Jan 26, 2022

vhirtham Jan 26, 2022 •

edited

Loading

CagtayFabry commented Jan 26, 2022

CagtayFabry commented Jan 26, 2022 •

edited

Loading

marscher commented Jan 26, 2022

vhirtham commented Jan 26, 2022

CagtayFabry commented Jan 26, 2022

CagtayFabry commented Jan 26, 2022 •

edited

Loading

vhirtham commented Jan 27, 2022

		>>> from weldx.core import GenericSeries
		>>> from weldx.constants import Q_

Fix failing tests and documentation #684

Fix failing tests and documentation #684

Conversation

vhirtham commented Jan 25, 2022 • edited Loading

Changes

Related Issues

Checks

codecov bot commented Jan 25, 2022 • edited Loading

Codecov Report

github-actions bot commented Jan 25, 2022 • edited Loading

Unit Test Results

review-notebook-app bot commented Jan 25, 2022

vhirtham commented Jan 25, 2022

marscher commented Jan 25, 2022

vhirtham commented Jan 26, 2022 • edited Loading

CagtayFabry Jan 26, 2022

Choose a reason for hiding this comment

vhirtham Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

CagtayFabry commented Jan 26, 2022

CagtayFabry commented Jan 26, 2022 • edited Loading

marscher commented Jan 26, 2022

vhirtham commented Jan 26, 2022

CagtayFabry commented Jan 26, 2022

CagtayFabry commented Jan 26, 2022 • edited Loading

vhirtham commented Jan 27, 2022

vhirtham commented Jan 25, 2022 •

edited

Loading

codecov bot commented Jan 25, 2022 •

edited

Loading

github-actions bot commented Jan 25, 2022 •

edited

Loading

vhirtham commented Jan 26, 2022 •

edited

Loading

vhirtham Jan 26, 2022 •

edited

Loading

CagtayFabry commented Jan 26, 2022 •

edited

Loading

CagtayFabry commented Jan 26, 2022 •

edited

Loading