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

overhaul Quick Start tutorial #134

Merged

Conversation

frx-wintermute
Copy link
Contributor

Hi, I would like to improve the Quick Start tutorial with the following changes.

Source files were added to automate the tutorial simulations and the visualization of results (with ParaView and LaTeX PGFPlots). This way, the runs may also be used as a sort of small regression test (for instance, to check whether a build of SU2 is able to work correctly).

Moreover, the structure of the tutorial text was reorganized.

Source files were added to automate the tutorial simulations and the
visualization of results (with ParaView and LaTeX PGFPlots).
This way the runs may also be used as a sort of small regression
test (for instance, to check whether a build of SU2 is able to
work correctly).

Moreover, the structure of the tutorial text was reorganized.
@pcarruscag
Copy link
Member

Thank you for taking the initiative on this.
We do not put mesh and solution files in the website repo.
For tutorials we use https://github.com/su2code/Tutorials
And to run automated regression tests on the tutorials we add them to this script https://github.com/su2code/SU2/blob/master/TestCases/tutorials.py (which is part of the code repo).
The website only links to the files stored in those repos.
Can you make those changes please?

@frx-wintermute
Copy link
Contributor Author

Hi @pcarruscag !

I am glad you like the initiative. I will make the changes you suggest, by preparing a pull request against https://github.com/su2code/Tutorials
Or maybe, for the QuickStart tutorial, I should use the https://github.com/su2code/SU2/tree/develop/QuickStart directory in the code repository?!? Please let me know which repository I should create a pull request against.

After that, I will of course re-adapt the proposed tutorial text, so that it refers to the files in the appropriate other repository. Should I amend the single commit of this pull request (so that the added files of Quick-Start_source/ do not even end up in the history of the branch to be merged into the https://github.com/su2code/su2code.github.io repository)? Or do you prefer a second commit that shows the changes made in response to your comment? What is the best strategy in your opinion?

@pcarruscag
Copy link
Member

You can add the files to QuickStart in that case the PR is to the SU2 code repo.
The scripts I would add to the Tutorials repo

@pcarruscag
Copy link
Member

You can continue on this branch, we can squash merge to avoid having the large files in the history

@frx-wintermute
Copy link
Contributor Author

You can add the files to QuickStart in that case the PR is to the SU2 code repo. The scripts I would add to the Tutorials repo

But in that case, we would have two files ('inv_NACA0012.cfg' and 'mesh_NACA0012_inv.su2') in two distinct repositories: one copy of the two files in the SU2 code repository and one copy (along with the other auxiliary files) in the Tutorials repository: I am afraid that the two copies would sooner or later get out of sync...

I would rather avoid such file duplication across distinct repositories.

I can imagine two possibile strategies:

(A) I make a pull request against the SU2 code repository, adding the auxiliary files to the subdirectory 'QuickStart/' (which already includes the files 'inv_NACA0012.cfg' and 'mesh_NACA0012_inv.su2'), and do not touch the Tutorials repository at all

(B) I make a pull request against the SU2 code repository, deleting the 'QuickStart/' subdirectory, and a pull request against the Tutorials repository, adding 'inv_NACA0012.cfg' and 'mesh_NACA0012_inv.su2' and the other auxiliary files to a suitable new subdirectory (for instance 'compressible_flow/QuickStart/')

Which one would you consider the best strategy? (A) or (B)?

@pcarruscag
Copy link
Member

B is what I had in mind in the previous reply

Those files have been moved to the Tutorials repository.
The tutorial text has been re-adapted accordingly.
@frx-wintermute
Copy link
Contributor Author

B is what I had in mind in the previous reply

OK, done!

Plan (B) consists of this pull request (with both the commits) and of the two other pull requests su2code/Tutorials#43 and su2code/SU2#2066

Please let me know, in case there's anything else to be fixed! ;-)

@pcarruscag
Copy link
Member

I'm very sorry, I had a fever yesterday and I didn't read your comment properly.

You can add the files to QuickStart in that case the PR is to the SU2 code repo.
The scripts I would add to the Tutorials repo

I meant to add the new config files to the QuickStart folder (code repo), and the scripts to create plots and figures to Tutorials.
We like to have the QuickStart case in the code repo because it can be run immediately, whereas for our other tests the meshes need to be obtained from somewhere else.
For the website (this PR) you link to all the files.
Apologies for the confusion.

@frx-wintermute
Copy link
Contributor Author

I'm very sorry, I had a fever yesterday and I didn't read your comment properly.

It's OK, no need to apologize: a fever can be very troublesome... I hope you feel better now! :-)

You can add the files to QuickStart in that case the PR is to the SU2 code repo.
The scripts I would add to the Tutorials repo

I meant to add the new config files to the QuickStart folder (code repo), and the scripts to create plots and figures to Tutorials. We like to have the QuickStart case in the code repo because it can be run immediately, whereas for our other tests the meshes need to be obtained from somewhere else. For the website (this PR) you link to all the files. Apologies for the confusion.

Well, I understand the usefulness of having a small case handy within the source code tree.

However, to be honest, I am not convinced that having auxiliary script files that depend on two essential files ('inv_NACA0012.cfg' and 'mesh_NACA0012_inv.su2'), but not those two essential files, in the Tutorials repository is a good idea. It would make a subdirectory in the Tutorials repository not self-sufficient... Those two essential files would need to be copied from another repository (the source code repository), and would sooner or later become inconsistent with what the auxiliary script files expect.

On the other hand, having those two essential files duplicated across two distinct repository does not seem to be a good thing, either, as I said before.

The more I think about it, the more I am convinced that my plan (A) is the way to go:

(A) I make a pull request against the SU2 code repository, adding the auxiliary files to the subdirectory 'QuickStart/' (which already includes the files 'inv_NACA0012.cfg' and 'mesh_NACA0012_inv.su2'), and do not touch the Tutorials repository at all

Please consider this possibility and tell me whether you are fine with it.

@pcarruscag
Copy link
Member

I understand but this is how we structured all other tutorials, the additional scripts are in the tutorials repo, you can place a readme file there pointing to the main tutorial on the website.
Also the regression tests in the code repo should include the tutorials (via the tutorials.py regression script) you can try to add a test for the scripts via that file.

@frx-wintermute
Copy link
Contributor Author

I understand but this is how we structured all other tutorials, the additional scripts are in the tutorials repo, you can place a readme file there pointing to the main tutorial on the website.

I am apparently misreading you somehow: it seems to me that you are saying that all the tutorials have additional scripts in the Tutorials repository, while the meshes and config files for those tutorials are in the SU2 code repository.

However, it seems to me that all the tutorials in the Tutorials repository are self-contained, with each subdirectory including meshes, config files, along with additional scripts, if any.

I am hesitant about scattering the Quick Start tutorial files across different repositories (mesh and config file in one repository, additional scripts in another repository). As I said before, it would make things somewhat fragile.

That's why, when you pointed out that you like to have the Quick Start tutorial handy within the code repository, I reiterated the proposal of the previously described plan (A). This keeps the Quick Start tutorial in the code repository, but along with its new additional files.

As an alternative, if the Quick Start mesh and basic config file must stay in the code repository, and no additional files can enter that same subdirectory at all, I can propose:

(C) I update the Quick Start config file in the code repository, and add a subdirectory to the Tutorials repository, including the additional scripts, but also a duplicate of the mesh and basic config file

I am not especially happy about file duplication across different repositories in plan (C), but I think it's anyway better than having a non-self-sufficient tutorial in the Tutorials repository. But I guess it's clear that I prefer plan (A)...

@pcarruscag
Copy link
Member

We will not duplicate anything and I do not want scripts / code that is not built / tested (regularly) in the code repo, not even in subdirs.
Please add the additional scripts to a sub dir in tutorials, and the required configs to the code repo, making sure that there are tests for them in su2code/TestCases/tutorials.py.
In the website you link to all the required files, it's just a link I dont see an issue with the files not being in the same repo. We have other examples of that.
Thanks.

@frx-wintermute
Copy link
Contributor Author

We will not duplicate anything and I do not want scripts / code that is not built / tested (regularly) in the code repo, not even in subdirs. Please add the additional scripts to a sub dir in tutorials, and the required configs to the code repo, making sure that there are tests for them in su2code/TestCases/tutorials.py. In the website you link to all the required files, it's just a link I dont see an issue with the files not being in the same repo. We have other examples of that. Thanks.

Other examples, such as?

I am asking, because I honestly cannot figure out how to do it right. I mean, how to do what you suggest, without creating the issues I have explained...

@pcarruscag
Copy link
Member

Most or all of the test cases https://github.com/su2code/TestCases only have meshes and the config files are in the code repo because when we need to update options as part of the PR it is easier this way.
Same for V&V https://github.com/su2code/VandV/tree/master/rans/30p30n
We have some tests that use a symlink to meshes in other directories to avoid duplicating them.
It's not an issue if not everything is in the same place.

@frx-wintermute
Copy link
Contributor Author

[...]

It's not an issue if not everything is in the same place.

I see: I have updated the pull requests, even though I still think that scattering files across different repositories may create trouble, sooner or later...

@pcarruscag pcarruscag merged commit 76b51c7 into su2code:develop Jul 15, 2023
@pcarruscag
Copy link
Member

Thank you

@frx-wintermute frx-wintermute deleted the improve_quickstart_tutorial branch July 21, 2023 21:13
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.

2 participants