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

Deprecate options that did not work since v7.0.0, and other minor cleanups #1129

Merged
merged 39 commits into from
Dec 14, 2020

Conversation

TobiKattmann
Copy link
Contributor

@TobiKattmann TobiKattmann commented Dec 10, 2020

Proposed Changes

Hi all,
Following a bit the idea of #942 this PR mostly gets rid of stuff.

  • WRT_RESIDUALS unused
  • WRT_LIMITERS unused
  • WRT_CON_FREQ_DUALTIME only used in legacy output ... so I thought I just go for it
  • removed SetMeshFile function as it not used anywhere and CSU2MeshFileWriter is the new cool kid on the block 🛴
  • Added some options to the config_template which were missing
  • Added output of the mpi rank to volume output of (in-)compressible and heat solver. I used that for debugging mostly but it might be interesting for others as well. Yes yes I know, one could think of putting COORDS, MESH_QUALITY (except for FEM) and now RANK into a centralized place to avoid duplication... next PR... I promise ... maybe
  • and a few tiny things
  • EDIT: yeah and some more options after my initial PR, see CConfig.cpp in files changed for all

There is a lot of other things to deprecate so feel free to add here.

I have to check the tutorials for these options as well... ✔️ done and open PR (see below) but not merged yet to wait for reviews. This fails to reg tests of tutorials.

(p.s. github now has a dark mode, which makes writing this on Thursday nights much nicer 🕶️)

Related Work

none

PR Checklist

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags, or simply --warnlevel=2 when using meson).
  • My contribution is commented and consistent with SU2 style.
  • I have added a test case that demonstrates my contribution, if necessary. not necessary
  • I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp) , if necessary. not necessary

@pcarruscag
Copy link
Member

Dark mode?
"Is it possible to learn this power?"

@TobiKattmann
Copy link
Contributor Author

Dark mode?
"Is it possible to learn this power?"

🧙 Sure, if you just go to your front page (i.e. just github.com) there should be a big button directly on the right side.
Or Settings->Appearance->Dark. Enjoy :)

@pcarruscag
Copy link
Member

What is the equivalent of DUAL_TIME in the TIME / OUTER / INNER versions we have now? TIME?
(so that we can auto-replace the stuff that is used in the testcases without breaking things too much)

With all this deprecation the next version number should probably be 7.1.0 even if I would call these things bug fixes from v6.2 to v7.0.
Going 7.0.8 -> 7.1.0 also saves the awkward 7.0.9 -> 7.1.0 (which would make people wonder if we know how version numbers work).

@TobiKattmann
Copy link
Contributor Author

Well thanks for fixing the typo.

It seems we use OUTPUT_WRT_FREQ for steady and unsteady simulations. Therefore the testcase cleanup was a bit more involved. For steady cases WRT_SOL_FREQ had to be replaced and for unsteady ones WRT_SOL_FREQ_DUALTIME to follow the author's intention... and I have the feeling that nearly every testcase had these options in it.

I have to admit I don't understand why 7.0.9 would be awkward 🤔

@TobiKattmann
Copy link
Contributor Author

I also removed your options from the tutorials @pcarruscag : only WRT_CSV_SOL was used in two cases

@pcarruscag
Copy link
Member

Because 7.0.9 + 1 = 7.0.10 xD
Thanks for taking care of the tutorials, I have a few more options coming.

Silly question, using OUTPUT_WRT_FREQ = 1 to output on every time iteration, will it cause outputs on every inner iteration for some unsteady problems that were using the DUALTIME options?
(I don't know much about unsteady options...)

@TobiKattmann
Copy link
Contributor Author

TobiKattmann commented Dec 11, 2020

Silly question, using OUTPUT_WRT_FREQ = 1 to output on every time iteration, will it cause outputs on every inner iteration for some unsteady problems that were using the DUALTIME options?
(I don't know much about unsteady options...)

For unsteady cases output is only written at the end of each "time"-iteration (if the OUTPUT_WRT_FREQ is matched), never inside the inner-iterations. OUTPUT_WRT_FREQ works differently for steady and unsteady. So that is not an issue.

@pcarruscag
Copy link
Member

pcarruscag commented Dec 11, 2020

@TobiKattmann are you going after WRT_CON_FREQ now? (or at some point)

@TobiKattmann
Copy link
Contributor Author

TobiKattmann commented Dec 11, 2020

I was too lazy on that. Sorry, I should have done that that in the first place 😞

As there were only a few non-defaults I would have gone with:
find . type f \( -name '*.cfg' \) -exec sed --in-place '/WRT_CON_FREQ= 1/d' {} \+
and changing the rest manually (exercising the muscle memory a bit), but that handy script might get some additional use for other options ;)

EDIT: This suggestion above is bad how I just noticed -> sed here deletes each line that contains WRT_CON_FREQ= 1 which rightfully contains WRT_CON_FREQ= 10 etc! One has to wrap the search expression in \b to only deletes lines that exactly match. The command becomes:
find . type f \( -name '*.cfg' \) -exec sed --in-place '/\bWRT_CON_FREQ= 1\b/d' {} \+

If you have some more options to deprecate in mind I can take care of some. Maybe I try to hunt down a few more over the weekend 🏹

@TobiKattmann
Copy link
Contributor Author

That seems to have confused git, maybe because of line endings on some testcases or something.

you prob mean that some files a "rewritten" completely. Hm seems to happen whenever a file ends on WRT_CONV_FREQ and does not have the default value.

bool output_per_surface = config->GetWrt_Surface();
bool output_per_surface = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Obviously this kind of change (there are just 3 in this file) takes away functionality from users of output_structure_legacy. In these cases I was always generous 👑 and enabled stuff which potentially clutters output but I am unsure whether anybody even uses this at this point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EDIT: now it is just WRT_SURFACE

@pcarruscag pcarruscag changed the title Minor cleanups / Remove deprecated options Deprecate options that did not work since v7.0.0, and other minor cleanups Dec 13, 2020
@TobiKattmann
Copy link
Contributor Author

Ok I am happy with the changes as they are right now. I've looked over the diff once again and I have no more comments (obviously more regarding @pcarruscag commits than my own). Btw thanks for being attentive regarding my mistakes and of course for all the added commits 👍

@pcarruscag
Copy link
Member

pcarruscag commented Dec 14, 2020

Thank you for taking the lead (this is boring stuff, but less so if more people do it).
I also cannot think of any more options, but I might try to do something about the objective functions.
EDIT: on another PR perhaps, so if want to hit that button...

@pcarruscag pcarruscag merged commit 2250751 into develop Dec 14, 2020
@pcarruscag pcarruscag deleted the chore_minor_cleanups branch December 14, 2020 22:31
@TobiKattmann
Copy link
Contributor Author

By the way, I noticed you didn't squash-merge. Any reasons or just forgotten? No big deal though

@pcarruscag
Copy link
Member

Cus yolo.
I have no idea how much that stuff matters to the repo size, so best not to prematurely optimize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants