[WIP] fix volume names #2079

bigfooted · 2023-07-12T04:52:02Z

Proposed Changes

For volume names, we assume that the filename is filename.ext and we strip the extension using find_last_of('.')
But:

This can lead to unwanted file stripping when you want the filename or folder to be volume.a
We already mention in the documentation that the volume filename should be given without an extension

I think we should simply remove the find_last_of lines. There are more of those, for now I have simply removed the one connected to #2074

Related Work

Issue: #2074

PR Checklist

I am submitting my contribution to the develop branch.
My contribution generates no new compiler warnings (try with --warnlevel=3 when using meson).
My contribution is commented and consistent with SU2 style (https://su2code.github.io/docs_v7/Style-Guide/).
I have added a test case that demonstrates my contribution, if necessary.
I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp), if necessary.

bigfooted · 2023-07-12T05:07:39Z

There are some other find_last_of lines as well, if all filenames are supposed to be given without any file extension, then these should all be removed.

pcarruscag · 2023-07-12T05:12:51Z

In principle sounds good, it will probably break lots of cases, optimization scripts and what not. We'll see, we are not leaving this inconsistent though, all or nothing.

bigfooted · 2023-07-12T06:13:38Z

In principle sounds good, it will probably break lots of cases, optimization scripts and what not. We'll see, we are not leaving this inconsistent though, all or nothing.

I agree.

…always the same Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

SU2_PY/SU2/io/tools.py

+        # restart = restart.split(".")[0]
+        # solution = solution.split(".")[0]
+
+        pass


To fix the issue, the pass statement on line 1181 should be removed. This will eliminate the unnecessary code without altering the functionality of the program. The block already contains valid statements, so removing pass will not affect the execution of the code.

Common/src/CConfig.cpp

pcarruscag · 2025-06-15T21:21:49Z

SU2_CFD/src/output/CMeshOutput.cpp

-  volumeFilename = config->GetMesh_Out_FileName();
+  volumeFilename = config->GetMesh_Out_FileName() + ".su2";



I don't like the hard-coded extensions everywhere.
We should encapsulate this name giving and extensions into the function to retrieve the name.
This encapsulation should also make it possible to handle names both with and without extensions, forcing everyone to remove extensions from their config is a large break of backwards compatibility and I don't want to release version 9 for something as trivial as one user complaining that the named without extension didn'e work...

So we accept "restart" as well as "restart.dat" etc...?
But we check if the extension is actually the correct one, so restart.csv will not be stripped, but seen as having 'no extension'?

Yes but that would be an user error so I think it's fine.

Common/src/grid_movement/CSurfaceMovement.cpp

pcarruscag

Our tests don't cover history files, breakdown files, etc. how do we know those are still output with the expected names?

Common/src/geometry/CPhysicalGeometry.cpp

pcarruscag · 2025-06-15T21:30:05Z

Common/src/grid_movement/CSurfaceMovement.cpp

      flowIter = nFlowIter - iter;
-      unsigned short lastindex = DV_Filename.find_last_of('.');
-      DV_Filename = DV_Filename.substr(0, lastindex);
      if ((SU2_TYPE::Int(flowIter) >= 0) && (SU2_TYPE::Int(flowIter) < 10))
        SPRINTF(buffer, "_0000%d.dat", SU2_TYPE::Int(flowIter));


pcarruscag · 2025-06-15T21:32:41Z

Common/src/grid_movement/CSurfaceMovement.cpp

    } else {
      /*--- Forward time for the direct problem ---*/
      flowIter = iter;
-      unsigned short lastindex = DV_Filename.find_last_of('.');
-      DV_Filename = DV_Filename.substr(0, lastindex);
      if ((SU2_TYPE::Int(flowIter) >= 0) && (SU2_TYPE::Int(flowIter) < 10))
        SPRINTF(buffer, "_0000%d.dat", SU2_TYPE::Int(flowIter));


same... this logic needs to be contained in helper functions, it will be a pain to get rid of the bugs this way... we'll have to reason about extensions in every area which is the opposite of what you are trying to achieve here I think.

pcarruscag · 2025-06-15T21:33:45Z

Common/src/grid_movement/CSurfaceMovement.cpp

    auto str = config[ZONE_0]->GetMesh_Out_FileName();
-    unsigned short lastindex = str.find_last_of('.');
-    str = str.substr(0, lastindex) + ".su2";
+    //unsigned short lastindex = str.find_last_of('.');
+    str += ".su2";


The function should return the name with extension and the callers of the function should not need to make further ad-hoc modifications

pcarruscag · 2025-06-15T21:34:21Z

SU2_CFD/src/drivers/CDriver.cpp

+
+      unsigned short lastindex = filename.find_last_of('.');
+      filename = filename.substr(0, lastindex);
+
      if (nZone > 1)
-        filename = config->GetMultizone_FileName(filename, iZone, ".dat");
+        filename = config->GetMultizone_FileName(filename, iZone, "");
+
+      /*--- At the moment we stay consistent with the default extension .dat for all input files. ---*/
+      filename += ".dat";


the function should handle the extension, not the client

pcarruscag · 2025-06-15T21:34:42Z

SU2_CFD/src/output/CFlowOutput.cpp

  auto fileName = config->GetBreakdown_FileName();
-  if (unsteady) {
-    const auto lastindex = fileName.find_last_of('.');
-    const auto ext = fileName.substr(lastindex, fileName.size());
-    fileName = fileName.substr(0, lastindex);
-    fileName = config->GetFilename(fileName, ext, curTimeIter);
-  }


seems wrong

pcarruscag · 2025-06-15T21:35:58Z

SU2_CFD/src/output/COutput.cpp


  string hist_ext = ".csv";
  if (config->GetTabular_FileFormat() == TAB_OUTPUT::TAB_TECPLOT) hist_ext = ".dat";



same, move the entire name logic into a helper function

pcarruscag · 2025-06-15T21:41:06Z

SU2_PY/SU2/io/state.py

+            direct_name = direct_name + ".csv"
+            adjoint_name = adjoint_name + ".csv"


the idea to keep things backwards compatible is:

Suggested change

direct_name = direct_name + ".csv"

adjoint_name = adjoint_name + ".csv"

direct_name = direct_name + ("" if direct_name.endswith(".csv") else ".csv")

adjoint_name = adjoint_name + ".csv"

bigfooted · 2025-06-15T21:44:59Z

SU2_CFD/src/output/COutput.cpp

  string hist_ext = ".csv";
  if (config->GetTabular_FileFormat() == TAB_OUTPUT::TAB_TECPLOT) hist_ext = ".dat";

-  /*--- Append the zone ID ---*/
+  /*--- Retrieve the history filename ---*/
+
+  historyFilename = config->GetConv_FileName();


@pcarruscag so we add the hist_ext stuff to GetConv_Filename?

Yeah that and any logic to suffix with number of iterations.

Whenever a given file name (like the history or breakdown) is only used for a particular type of file, the accessor to get the file name can return it with extension.
The for output files whose extension depends on the type of format being output, we can add an accessor per file type, or make it clear in the name accessor that the name is without extension. However, it should contain everything else in the name that we need, zone idx, time step, etc.
I think this is the safest way to make this change while providing backward compatibility easily in one place.

ok,
I move the entire filename construction into GetConv_FileName.
I strip ".dat" and ".csv" only and keep other extensions. Users can still use "history.dat", but using "history" or "history.1.2.3.old" will now work.

Common/src/grid_movement/CSurfaceMovement.cpp

Common/include/CConfig.hpp

Common/src/grid_movement/CSurfaceMovement.cpp

SU2_CFD/src/output/filewriter/CSU2MeshFileWriter.cpp

SU2_CFD/src/solvers/CBaselineSolver_FEM.cpp

SU2_CFD/src/solvers/CSolver.cpp

+  //const auto iZone = config->GetiZone();
+  //const auto nZone = config->GetnZone();


To fix the issue, we need to either remove the commented-out code entirely or reinstate it into the active codebase. Since the purpose of the commented-out code is unclear from the provided snippet, the best approach is to remove it to improve code clarity and maintainability. If the code is needed in the future, it can be retrieved from version control.

The specific lines to address are:

//const auto iZone = config->GetiZone();

//const auto nZone = config->GetnZone();

These lines should be removed from the file SU2_CFD/src/solvers/CSolver.cpp.

Common/src/CConfig.cpp

SU2_CFD/src/solvers/CIncEulerSolver.cpp

-
-    if (nZone > 1) filename_ = config->GetMultizone_FileName(filename_, iZone, ".dat");
+  //   // nijso: this is inconsistent with line 70, nZone<= 1!
+    if (nZone > 1) filename_ = config->GetMultizone_FileName(filename_, iZone, "");


To fix the issue, the redundant comparison if (nZone > 1) on line 76 should be removed. Since the condition nZone <= 1 is already enforced by the outer if statement on line 70, the block of code dependent on nZone > 1 will never execute. Removing this comparison will eliminate the logical inconsistency and improve code clarity.

bigfooted added 2 commits July 9, 2023 15:31

fix volume names

19fc59d

Merge branch 'develop' into fix_volume_names

9ef203f

bigfooted added the changelog:fix label Jul 12, 2023

pcarruscag changed the title ~~fix volume names~~ [WIP] fix volume names Jul 13, 2023

bigfooted added 23 commits July 19, 2023 10:59

Merge branch 'develop' into fix_volume_names

59a0f58

Merge branch 'develop' into fix_volume_names

627356e

Merge branch 'develop' into fix_volume_names

6ce0cce

Merge branch 'develop' into fix_volume_names

2a0e7db

Merge branch 'develop' into fix_volume_names

32ac2ea

Merge branch 'develop' into fix_volume_names

3b8bcd5

Merge branch 'develop' into fix_volume_names

4f87d14

Merge branch 'develop' into fix_volume_names

4269a4c

Merge branch 'develop' into fix_volume_names

cdbcb43

remove other last_of occurrences

3fa684d

Merge branch 'develop' into fix_volume_names

92dacb0

Merge branch 'develop' into fix_volume_names

0e729fa

Merge branch 'develop' into fix_volume_names

a251a58

Merge branch 'develop' into fix_volume_names

df9aecb

Merge branch 'develop' into fix_volume_names

cba0317

Merge branch 'develop' into fix_volume_names

f4bf669

Merge branch 'develop' into fix_volume_names

b119bcb

Merge branch 'develop' into fix_volume_names

6632ade

Merge branch 'develop' into fix_volume_names

bde4228

Merge branch 'develop' into fix_volume_names

5c08794

Merge branch 'develop' into fix_volume_names

a4f031e

Merge branch 'develop' into fix_volume_names

024a0a2

remove suffix from vandv

e351550

Potential fix for code scanning alert no. 5430: Comparison result is …

078bd4a

…always the same Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

github-advanced-security bot found potential problems Jun 13, 2025

View reviewed changes

final fix and cleanup

dfd533a

github-advanced-security bot found potential problems Jun 13, 2025

View reviewed changes

Common/src/CConfig.cpp Fixed Show fixed Hide fixed

bigfooted added 4 commits June 14, 2025 12:04

small fix for default_properties

2fa6e1f

small fix for default_properties

505b824

small fix for default_properties

b3d4008

remove suffix from mesh_out

5c0c771

pcarruscag reviewed Jun 15, 2025

View reviewed changes

github-advanced-security bot found potential problems Jun 15, 2025

View reviewed changes

Common/src/grid_movement/CSurfaceMovement.cpp Fixed Show fixed Hide fixed

pcarruscag reviewed Jun 15, 2025

View reviewed changes

bigfooted commented Jun 15, 2025

View reviewed changes

bigfooted added 3 commits June 17, 2025 10:41

put entire history filename construction into getconv_filename

5677a38

put entire history filename construction into getconv_filename

6560ebb

clang-format

b06593c

github-advanced-security bot found potential problems Jun 17, 2025

View reviewed changes

Common/src/grid_movement/CSurfaceMovement.cpp Fixed Show fixed Hide fixed

bigfooted added 3 commits June 18, 2025 00:07

type mismatch

80aee33

fix typo

36cecee

move dealing with extensions up to the getfile functions

239bd59

github-advanced-security bot found potential problems Jun 26, 2025

View reviewed changes

bigfooted added 2 commits June 27, 2025 11:57

precommit

c11b8a8

remove warning

623a74a

github-advanced-security bot found potential problems Jun 27, 2025

View reviewed changes

bigfooted added 3 commits June 28, 2025 16:39

correct some extension

07d6422

simplify adding leading zeroes

3318807

merge develop

785edae

github-advanced-security bot found potential problems Jun 29, 2025

View reviewed changes

Common/src/CConfig.cpp Fixed Show fixed Hide fixed

update FEA_Filename

c603132

github-advanced-security bot found potential problems Jun 29, 2025

View reviewed changes

update dynamics filename

a84e5ea

@@ -1180,3 +1180,2 @@
                     solution = config.SOLUTION_FILENAME
-                    pass

		volumeFilename = config->GetMesh_Out_FileName();
		volumeFilename = config->GetMesh_Out_FileName() + ".su2";


		string hist_ext = ".csv";
		if (config->GetTabular_FileFormat() == TAB_OUTPUT::TAB_TECPLOT) hist_ext = ".dat";

		direct_name = direct_name + ".csv"
		adjoint_name = adjoint_name + ".csv"

		//const auto iZone = config->GetiZone();
		//const auto nZone = config->GetnZone();

[WIP] fix volume names #2079

Are you sure you want to change the base?

[WIP] fix volume names #2079

Conversation

bigfooted commented Jul 12, 2023

Proposed Changes

Related Work

PR Checklist

Uh oh!

bigfooted commented Jul 12, 2023

Uh oh!

pcarruscag commented Jul 12, 2023

Uh oh!

bigfooted commented Jul 12, 2023

Uh oh!

Check warning

Copilot Autofix

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pcarruscag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check notice

Copilot Autofix

Uh oh!

Uh oh!

Check warning

Copilot Autofix

Uh oh!