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

switching this lesson to the new Carpentries Workbench Template #23

Merged
merged 70 commits into from
Oct 8, 2024

Conversation

razoumov
Copy link
Member

As discussed at #22, doing this via a PR.

@reid-a
Copy link
Member

reid-a commented Sep 2, 2024

Assignment received, will try to find some bandwidth for this soon.

@tkphd tkphd requested review from tkphd and reid-a September 5, 2024 11:29
Copy link
Member

@tkphd tkphd left a comment

Choose a reason for hiding this comment

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

Thank you @razoumov, it's great to see activity on Chapel! It hasn't been updated in a while, and I have avoided scrutinizing since there has not been an active maintainer for several years. My review comments apply to the Workbench conversion (which seems fine), and some edits to the old and revised content for clarity of exposition. I appreciate your efforts on this material.

CITATION.cff Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
FIXME.Rproj Outdated Show resolved Hide resolved
LICENSE.md Show resolved Hide resolved
episodes/01-intro.md Outdated Show resolved Hide resolved
episodes/13-synchronization.md Outdated Show resolved Hide resolved
episodes/14-parallel-case-study.md Outdated Show resolved Hide resolved
episodes/14-parallel-case-study.md Outdated Show resolved Hide resolved
episodes/14-parallel-case-study.md Outdated Show resolved Hide resolved
learners/setup.md Show resolved Hide resolved
@tkphd
Copy link
Member

tkphd commented Sep 14, 2024 via email

Alex Razoumov added 21 commits September 16, 2024 08:59
… instructions are more of a placeholder for someone to test
LICENSE.md Outdated Show resolved Hide resolved
Copy link
Member

@tkphd tkphd left a comment

Choose a reason for hiding this comment

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

Good progress so far! Thanks, @razoumov

Copy link
Member

@tkphd tkphd left a comment

Choose a reason for hiding this comment

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

Excellent edit to ep. 13!

@razoumov
Copy link
Member Author

razoumov commented Oct 1, 2024

Hi Trevor @tkphd - I believe I went through all the points in your review, closed the ones that have been resolved through a commit, and commented on the rest. At this point my preference would be to merge this PR into the repo, to publish in the new workbench format, and open new issues for any unresolved points in this PR if needed. I know there is an ongoing discussion about using Quarto over the Varnish engine (looking at the meeting agenda, have not attended the meetings for a while). If there is an organization-wide HPC Carpentry transition to Quarto, I am happy to adopt it for HPC Chapel in a future PR (that someone more familiar with Quarto should create), but at this point I would prefer to merge the already committed extensive set of changes into the repo. What are your thoughts?

Copy link
Member

@tkphd tkphd left a comment

Choose a reason for hiding this comment

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

I don't think the LICENSE needs a separate section for software, as there are no software source code artifacts in the repository, only code-blocks in the Markdown-formatted plain text. This falls under normal copyright, not software licensing, but is also not hugely important.

- "Know how to define and use data stored as variables."
::::::::::::::::::::::::::::::::::::::::::::::::

Using basic maths in Chapel is fairly intuitive. Try compiling the following code to see
Copy link
Member

Choose a reason for hiding this comment

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

Contextualize this for beginners, like the Ep. 1 examples:
Open a text editor and populate the document with these lines; save it as operators.chpl; compile and run with the following command.

It can span as many lines as you want!
(like this) */
```

Copy link
Member

Choose a reason for hiding this comment

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

Better practice would make that edit, or create a block comment describing what the code does in operators.chpl, then recompile & run. This will catch a few learners' typos.

Comment on lines +119 to +122




Copy link
Member

Choose a reason for hiding this comment

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

Too many blank lines.


```chpl
const test = 100;
writeln('The value of test is ', test, ' and its type is ', test.type:string);
Copy link
Member

Choose a reason for hiding this comment

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

Neato, and please talk through the syntax of test.type:string

@tkphd tkphd merged commit 89c58a3 into main Oct 8, 2024
2 checks passed
@tkphd tkphd deleted the dev-workbench branch October 8, 2024 19:14
github-actions bot pushed a commit that referenced this pull request Oct 8, 2024
Auto-generated via {sandpaper}
Source  : 89c58a3
Branch  : main
Author  : Trevor Keller <[email protected]>
Time    : 2024-10-08 19:13:39 +0000
Message : Merge pull request #23 from hpc-carpentry/dev-workbench

switching this lesson to the new Carpentries Workbench Template
github-actions bot pushed a commit that referenced this pull request Oct 8, 2024
Auto-generated via {sandpaper}
Source  : cf65a56
Branch  : md-outputs
Author  : GitHub Actions <[email protected]>
Time    : 2024-10-08 19:15:25 +0000
Message : markdown source builds

Auto-generated via {sandpaper}
Source  : 89c58a3
Branch  : main
Author  : Trevor Keller <[email protected]>
Time    : 2024-10-08 19:13:39 +0000
Message : Merge pull request #23 from hpc-carpentry/dev-workbench

switching this lesson to the new Carpentries Workbench Template
@tkphd
Copy link
Member

tkphd commented Oct 8, 2024

@razoumov I merged this PR, and the build "succeeded," but the rendered site does not look correct:
https://www.hpc-carpentry.org/hpc-chapel/

@razoumov
Copy link
Member Author

razoumov commented Oct 9, 2024

Thank you @tkphd for merging this pull request! I restarted the Pages built process (switched the site source to None and then back to gh-pages), and it now built the website properly https://www.hpc-carpentry.org/hpc-chapel

@tkphd
Copy link
Member

tkphd commented Oct 9, 2024

Excellent! Thanks @razoumov

github-actions bot pushed a commit that referenced this pull request Oct 15, 2024
Auto-generated via `{sandpaper}`
Source  : cf65a56
Branch  : md-outputs
Author  : GitHub Actions <[email protected]>
Time    : 2024-10-08 19:15:25 +0000
Message : markdown source builds

Auto-generated via {sandpaper}
Source  : 89c58a3
Branch  : main
Author  : Trevor Keller <[email protected]>
Time    : 2024-10-08 19:13:39 +0000
Message : Merge pull request #23 from hpc-carpentry/dev-workbench

switching this lesson to the new Carpentries Workbench Template
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.

3 participants