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

Second tutorial about time dependent data #564

Merged
merged 31 commits into from
Nov 3, 2021

Conversation

vhirtham
Copy link
Collaborator

@vhirtham vhirtham commented Sep 27, 2021

Changes

Add the second tutorial about time-dependent data

Todo

  • Link first tutorial
  • Add to "Getting started"
  • xarray examples -> mean, min, max

@vhirtham vhirtham added the documentation Improvements or additions to documentation label Sep 27, 2021
@vhirtham vhirtham self-assigned this Sep 27, 2021
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@vhirtham
Copy link
Collaborator Author

Notes

  • plot first
  • small data processing example (mean value)

@CagtayFabry
Copy link
Member

can you update the single_pass_weld schema in a separate PR to point to TimeSeries @vhirtham ?

Or maybe we should allow both for now?

@vhirtham vhirtham marked this pull request as ready for review October 1, 2021 07:55
@vhirtham
Copy link
Collaborator Author

vhirtham commented Oct 1, 2021

can you update the single_pass_weld schema in a separate PR to point to TimeSeries @vhirtham ?

Or maybe we should allow both for now?

I will create a separate PR pointing to the master branch

@vhirtham
Copy link
Collaborator Author

vhirtham commented Oct 1, 2021

@marscher @CagtayFabry no clue why deepsource is complaining here. The function is in the file and I didn't touch it either. Any clue? If not I will ignore it for now. If it keeps showing up, I will add a flag for deepsource to ignore it, before we merge everything into master.

@codecov
Copy link

codecov bot commented Oct 1, 2021

Codecov Report

Merging #564 (1897206) into new_tutorials (b2a3920) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##           new_tutorials     BAMWelDX/weldx#564   +/-   ##
==============================================
  Coverage          94.65%   94.65%           
==============================================
  Files                 93       93           
  Lines               5936     5936           
==============================================
  Hits                5619     5619           
  Misses               317      317           

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 b2a3920...1897206. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Oct 14, 2021

Unit Test Results

       1 files  ±0         1 suites  ±0   2m 4s ⏱️ +13s
1 923 tests ±0  1 923 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 1897206. ± Comparison against base commit b2a3920.

♻️ This comment has been updated with latest results.

@vhirtham vhirtham requested a review from CagtayFabry October 25, 2021 15:04
@vhirtham
Copy link
Collaborator Author

One bad thing about using RST: The link syntax collides with the markdown inline code syntax. See "Further readings" section here ... I'll try using HTML as an alternative

@CagtayFabry
Copy link
Member

One bad thing about using RST: The link syntax collides with the markdown inline code syntax. See "Further readings" section here ... I'll try using HTML as an alternative

I guess you meant this snippet?

[API tutorial: `TimeSeries`](timeseries_01.ipynb)`

I can see how that could be troublesome for the parser, but a normal link without inline code styling should also be fine here :)

@vhirtham
Copy link
Collaborator Author

vhirtham commented Oct 26, 2021

[API tutorial: `TimeSeries`](timeseries_01.ipynb)`

I can see how that could be troublesome for the parser, but a normal link without inline code styling should also be fine here :)

That works, yes.

I tried this one now:

[API tutorial: <code>TimeSeries</code>](timeseries_01.ipynb)

Works in the NBs and it should also work in the online version. We will know in a few minutes 😉

@vhirtham
Copy link
Collaborator Author

We will know in a few minutes 😉

Works (good enough).

Comment on lines +102 to +105
"Depending on the Python environment you are using, there are some things to consider.\n",
"Since the `plot` method uses `matplotlib` internally, you have to call the `matplotlib.pyplot.show` function manually after using `plot` when running a regular Python script file.\n",
"Otherwise, the plot won't show up.\n",
"\n",
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 prefer to leave this part out since we are in a notebook here, talking matplotlib or even other IDEs is confusing imo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If somebody reads our online doc, he is not using any environment at all. I know, I keep repeating myself, but I don't think it is wise to expect that every future WelDX user will also use Jupyter.

So IMO 3 sentences to avoid confusion beneath the Python userbase isn't too confusing for other people. Also, even if the tutorials are written in Jupyter, they are general WelDX tutorials and not "WelDX in Jupyter" tutorials.

As an alternative I can shorten it to a Hint like this:

HINT: In a Python script, you need to call matplotlib.pyplot.show function manually to actually see the plots

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussed, we will leave it in for now

Comment on lines +111 to +118
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"# %matplotlib widget"
]
Copy link
Member

Choose a reason for hiding this comment

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

can we uncomment this cell and hide+skip in the docs build?

Copy link
Collaborator Author

@vhirtham vhirtham Oct 27, 2021

Choose a reason for hiding this comment

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

I think hiding it in the docs doesn't make much sense when the preceding sentence is:

To enable interactive matplotlib plots inside a jupyter notebook, uncomment and run the following magic command:

However, I can try uncommenting it and we will see what happens. Maybe it magically works.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn't work. Solution postponed

@vhirtham vhirtham merged commit 37d389e into BAMWelDX:new_tutorials Nov 3, 2021
@vhirtham vhirtham deleted the simple_data branch November 3, 2021 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants