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 tutorials #3880

Merged
merged 16 commits into from
Nov 6, 2023
Merged

Add tutorials #3880

merged 16 commits into from
Nov 6, 2023

Conversation

spyridon97
Copy link
Contributor

This PR fixes several documentation warnings, and adds tutorials for the basic ADIOS concepts

This PR fixes #3793.

Copy link
Contributor

@anagainaru anagainaru left a comment

Choose a reason for hiding this comment

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

Two questions/comments

  1. You moved a bunch of examples from bpWriteRead{name} to bpTimeWriteRead{name}. What is the significance of Time in the name? I would keep the name the way it was just because it was shorter and still intuitive (unless there is a reason for the Time addition).
  2. You added a bunch of skeleton codes (which is great for the tutorial text you have). I would place them all in a separate folder instead of the hello

I left a bunch of comments with a few typos, but other than this it looks really good. Good job !

docs/user_guide/source/tutorials/attributes.rst Outdated Show resolved Hide resolved
docs/user_guide/source/tutorials/attributes.rst Outdated Show resolved Hide resolved
docs/user_guide/source/tutorials/attributes.rst Outdated Show resolved Hide resolved
docs/user_guide/source/tutorials/helloWorld.rst Outdated Show resolved Hide resolved
docs/user_guide/source/tutorials/timesteps.rst Outdated Show resolved Hide resolved
@spyridon97
Copy link
Contributor Author

  1. You moved a bunch of examples from bpWriteRead{name} to bpTimeWriteRead{name}. What is the significance of Time in the name? I would keep the name the way it was just because it was shorter and still intuitive (unless there is a reason for the Time addition).

I did it because bpWriteRead is kinda very vague, and there are a bunch of examples like that. This particular examples show how to write and read time steps. And that's how i also use them.

  1. You added a bunch of skeleton codes (which is great for the tutorial text you have). I would place them all in a separate folder instead of the hello

@vicentebolea and I decided that the best place to put them is next to the actual code so that we don't forget to update them, if needed.

@eisenhauer
Copy link
Member

I did it because bpWriteRead is kinda very vague, and there are a bunch of examples like that. This particular examples show how to write and read time steps. And that's how i also use them.

I'd use "Step" rather than "Time" in the name. (In general I think we've been trying to move away from the "timestep" nomenclature, dropping time and keeping step.

@spyridon97
Copy link
Contributor Author

I did it because bpWriteRead is kinda very vague, and there are a bunch of examples like that. This particular examples show how to write and read time steps. And that's how i also use them.

I'd use "Step" rather than "Time" in the name. (In general I think we've been trying to move away from the "timestep" nomenclature, dropping time and keeping step.

So you would prefer to rename the examples to bpStepWriteRead*?

And is it okay if i stick to timesteps for the name of the particular tutorial?

@eisenhauer
Copy link
Member

So you would prefer to rename the examples to bpStepWriteRead*?
Definitely would be my preference. "Time" doesn't appear in our APIs, but "Step" is prominent. ("Timestep" is in some SST engine parameters, but we likely should change that at some point.)

And is it okay if i stick to timesteps for the name of the particular tutorial?
I'd ask @pnorbert to weigh in on that. I know about the de-emphasis on Timestep in favor of Step, but not as much about the why's. Norbert has done the demo and tutorials enough to opine on this.

@spyridon97
Copy link
Contributor Author

spyridon97 commented Oct 31, 2023

So you would prefer to rename the examples to bpStepWriteRead*?
Definitely would be my preference. "Time" doesn't appear in our APIs, but "Step" is prominent. ("Timestep" is in some SST engine parameters, but we likely should change that at some point.)

And is it okay if i stick to timesteps for the name of the particular tutorial?
I'd ask @pnorbert to weigh in on that. I know about the de-emphasis on Timestep in favor of Step, but not as much about the why's. Norbert has done the demo and tutorials enough to opine on this.

I decided to remove the word time from everywhere in this set of tutorials, I hope this change addresses all of your concerns.

@spyridon97 spyridon97 force-pushed the add-tutorials branch 7 times, most recently from 0806c64 to 6e1538a Compare October 31, 2023 17:27
@vicentebolea
Copy link
Collaborator

The crusher failure can be resolve if you "update the branch", IMPORTANT use rebase update

@vicentebolea
Copy link
Collaborator

Screenshot from 2023-11-01 12-35-21

@spyridon97
Copy link
Contributor Author

@vicentebolea the bpls results were added, can you check them and approve this PR?

Copy link
Collaborator

@vicentebolea vicentebolea left a comment

Choose a reason for hiding this comment

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

The changes with regards the bpls usage looks good. 👍

As for the the several changes in the documentation unrelated to the tutorials, they look good in a first glance, however, while it is great to improve things here and there, it is best to do it in a different PR. The reason for this is that opening many fronts will both slow the review process and confuse reviewers.

@spyridon97
Copy link
Contributor Author

The changes with regards the bpls usage looks good. 👍

As for the the several changes in the documentation unrelated to the tutorials, they look good in a first glance, however, while it is great to improve things here and there, it is best to do it in a different PR. The reason for this is that opening many fronts will both slow the review process and confuse reviewers.

I will keep that in mind for future PRs.

Copy link
Collaborator

@vicentebolea vicentebolea 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, couple of comments, let the CI pass before making change so we can see the crusher CI result

docs/user_guide/source/advanced/query.rst Outdated Show resolved Hide resolved
docs/user_guide/source/components/engine.rst Show resolved Hide resolved
docs/user_guide/source/ecosystem/h5vol/vol.rst Outdated Show resolved Hide resolved
docs/user_guide/source/advanced/ecp_hardware.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@vicentebolea vicentebolea left a comment

Choose a reason for hiding this comment

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

Upon CI passing

@spyridon97
Copy link
Contributor Author

Upon CI passing

There seems to be unrelated tests failing. Shall i merge?

@vicentebolea
Copy link
Collaborator

Indeed, it is unrelated, ready to merge!

@spyridon97 spyridon97 merged commit 2ab1124 into ornladios:master Nov 6, 2023
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Tutorial section at the ADIOS2 readthedocs
4 participants