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

Discussion: rename executable and python scripts #83

Closed
davidscn opened this issue Feb 10, 2022 · 19 comments · Fixed by #131
Closed

Discussion: rename executable and python scripts #83

davidscn opened this issue Feb 10, 2022 · 19 comments · Fixed by #131

Comments

@davidscn
Copy link
Member

The main executable of aste is currently called preciceMap. As pointed out by @uekerman, this name does not really reflect what the executable does, namely feeding data into preCICE. We should rename it to something more expressive.

Similarly, we might want to adapt the names of the python scripts. We have the vtk_calculator.py, which is actually used in order to generate, compare and evaluate data on meshes, the partition_mesh.py which partitions the meshes and join_mesh.py in order to join them together again. Maybe it would even be useful to merge all python scripts into a single script in order to have a better overview.

Current suggestions I remember are for preciceMap:

  1. aste
  2. aste-core
@kursatyurt
Copy link
Collaborator

From to alternatives I am up for aste-core. I would also suggest ASTE Artifical Solver : aste-arso. For the scripts, we may also name them as aste-calculator.py aste-partitioner.py aste-joiner.py.

@fsimonis
Copy link
Member

The aste- prefix is generally a good idea as it prevents conflicts.

I personally prefer verbs or names as application names.

aste-join
aste-partition
aste-create

The main mapping utility is tricky though.

The current use is to feed meshes and point data into precice.
The new replay functionality will allow to imitate a solver.

The following words come to my mind

feed
io
interface
minimal
mimic
pretend
funnel
simulate (simu) 
abstract solver (arso) 
mimimal solver (miso)

I personally like aste-miso

@uekerman
Copy link
Member

I like

aste-join
aste-partition
aste-create

What about aste-run for preciceMap?

@davidscn
Copy link
Member Author

It might be a good idea to decide on this before I present my quick demo during the workshop. I would also vote for aste-join and aste-partition (maybe even merge both into a single script in the future?). For preciceMap I like aste-run or aste-simulate.
However, the vtk_calculator.py is responsible for data pre- and postprocessing. I think aste-create is a bit tied to the preprocessing step. Maybe aste-calculate, aste-compute or aste-evaluate would fit better.

@IshaanDesai
Copy link
Member

Upon request from @davidscn here are my suggestions as somebody who does not know the inner workings of ASTE and is looking at this discussion purely from a novice user perspective:

  • As the tool is called aste, having an executable aste makes sense. Executables oftentimes carry only the name of the tool and it would make it clear to a user on which file to run.
  • The files vtk_calculator.py, partition_mesh.py and join_mesh.py seem to have tools which aste uses, so perhaps it would be a good idea to merge these files and have callable function from there. This merged script can be called aste-core.py from which functionality is imported into the main script. For now having aste- prefixes to each of these files seem reasonable.

@BenjaminRodenberg
Copy link
Member

I also generally like the aste- prefix, if scripts are required. I don't know the aste architecture well, but I was always wondering why there is not a single executable which is then calling the subroutines. I imagine being able to type aste --help as a starting point and then aste run ... would be nice (similar to git add ...), but is maybe also totally out of scope.

It might also help to take a look at https://github.com/precice/aste/blob/develop/contrib/demo.sh and try to translate it to the new names. Currently, it is hard to see which commands are ASTE commands and which are not. That preciceMap and join_mesh.py somehow belong to the same family is not really clear.

Another example workflow of aste that might be instructive is the replay mode. How would the replay mode look like, if you map it to the new names?

@MakisH
Copy link
Member

MakisH commented Feb 17, 2022

Looking at the demo, I guess that these names would be clear:

  • aste-evaluate: Evaluate a function on a mesh
  • aste-partition: Partition a mesh
  • aste-run: Run ASTE, which calls preCICE (a bit strange since the other executables are also "run", but also ok since it is the main tool)
  • aste-join: Join a partitioned mesh

I would also prefer the "one executable -- multiple commands" scheme (aste <command>), but I understand this may be complicated. In any case, it would then be relatively easy to switch in the future.

@kursatyurt
Copy link
Collaborator

Thank you very much for all your contributions :)

Since tools are not a core part of ASTE and are written in a different language than core I would keep them separate and write a wrapper around them so that users can access all of them using one command.

aste-tools.py evalalute --mesh "mesh.vtk" --function "sqrt(x^2+y^2+z^2)" --data "Distance"  
aste-tools.py partitition --mesh "mesh.vtu" -n 2 --algorithm "topology"
aste-tools.py join --mesh "partitioned_mesh" 

Another example workflow of aste that might be instructive is the replay mode. What would the replay mode look like, if you map it to the new names?

For the replay mode, we don't need scripts. Probably everything will be settled in configuration file #85 #84. After renaming we just need to give a configuration file.

aste-run -c "FluidSolver.yml" &
aste-run -c "SolidSolver.yml"

then it re-produces the case for us for diagnosis or adapter development.

@MakisH
Copy link
Member

MakisH commented Feb 18, 2022

I like aste-tools.py <command> and aste-run!

@fsimonis
Copy link
Member

Merging all scripts into a single one has some downsides. I would vouch for multiple prefixed scripts.

  1. It makes individual modifications more difficult. Let's assume we need to migrate the partitioner to C++ for performance reasons, then we have a mix of languages dispatched by a single script. This will be either ugly or complicated (pybind, cython, dlload)

  2. Merging the commands will either lead to a huge script, or many separate scripts, which need to be available from a dispatch script.

  3. Aste run will be a tool separate of aste-tools, but they are all tools. This would confuse me as a user.

  4. Seeing aste-tools still requires to execute the command to see all subcommands (unless we want to support one autocompletion script per shell). Individual scripts allow to simply TAB complete the actual command of choice.

@kursatyurt
Copy link
Collaborator

Merging all scripts into a single one has some downsides. I would vouch for multiple prefixed scripts.

  1. It makes individual modifications more difficult. Let's assume we need to migrate the partitioner to C++ for performance reasons, then we have a mix of languages dispatched by a single script. This will be either ugly or complicated (pybind, cython, dlload)

IDK if it is possible to pass all arguments from Python to a C++ binary using subprocess or something like that but it works pretty easy in Python as tools act as a module.

  1. Merging the commands will either lead to a huge script, or many separate scripts, which need to be available from a dispatch script.

I agree that to avoid a huge script file I kept them as separate modules see #86.

  1. Aste run will be a tool separate of aste-tools, but they are all tools. This would confuse me as a user.

I disagree with that since aste-run is actually the core part and does not rely on aste-tools. It is more like aste-run is the main tool but aste-tools is a collection of post/pre-processing tools for ASTE.

  1. Seeing aste-tools still requires to execute the command to see all subcommands (unless we want to support one autocompletion script per shell). Individual scripts allow to simply TAB complete the actual command of choice.

TAB completion does not work either for individual scripts or the core part of ASTE now. For python script it requires additional package dependency, also requires additional work for C++ I am not sure we even need this. The only downside is --help for now since it is directly captured by argparse requires a workaround to give help messages for subtools.

@fsimonis
Copy link
Member

I agree that to avoid a huge script file I kept them as separate modules see #86.

Problem is that these individual modules need to be available to the launcher script.
This can be either in the same directory or in a directory visible to the python importer.
Installing them in the same directory is not an option, as all modules will also be visible to the the user as executables. This will lead to confusion once the user uses TAB-completion:

$ aste<TAB>
astePartition.py
asteJoin.py
aste-tools

An alternative would be to install them in a place respected by the python importer.
Looking at the system path raises the question on how we implement this portably in CMake without having to involve a package manager such as pip.

$ python -c "import sys; list(map(print,enumerate(sys.path)))"

I disagree with that since aste-run is actually the core part and does not rely on aste-tools. It is more like aste-run is the main tool but aste-tools is a collection of post/pre-processing tools for ASTE.

From the developer-perspective, aste-run is the core part as is the component that uses preCICE.
The user-perspective is different though. ASTE provides tools to test/replay a scenario with preCICE. The user needs to use tools one after the other to setup, execute and analyze the test.
Hence, from a user-perspective all aste-provided tools are tools.

TAB completion does not work either for individual scripts or the core part of ASTE now. For python script it requires additional package dependency, also requires additional work for C++ I am not sure we even need this. The only downside is --help for now since it is directly captured by argparse requires a workaround to give help messages for subtools.

I was referring to TAB completion of the commands, not their arguments. This is provided by most shells out-of-the-box.

$ aste-<TAB>
aste-join
aste-partition
aste-run

My recommendation is the following: Let's keep it simple and flexible by using separate prefixed scripts.

@kursatyurt
Copy link
Collaborator

Problem is that these individual modules need to be available to the launcher script. This can be either in the same directory or in a directory visible to the python importer. Installing them in the same directory is not an option, as all modules will also be visible to the user as executables. This will lead to confusion once the user uses TAB-completion:

If the modules don't have executable permission they don't appear in autocomplete phase. However, this is not the cleanest solution obviously.

@davidscn
Copy link
Member Author

I'm also a bit hesitant to merge now everything into one executable. First, I don't see a real advantage of typing aste-tools.py evaluate over aste-evaluate and second, the partitioning and joining are functional more related to the aste-run instead of the tools, see also the open issue #45. A long term goal would be to integrate the partitioning and joining functionality into the aste-run executable.

Considering that the opinions here are quite different, maybe let's discuss this again in the beginning of March in person.

@kursatyurt
Copy link
Collaborator

kursatyurt commented Aug 10, 2022

A general summary;

Current Tools names:

  • vtk_calculator.py
  • partition_mesh.py
  • join_mesh.py
  • preciceMap

Suggested changes are :

  • aste-evaluate: Evaluate a function on a mesh
  • aste-partition: Partition a mesh
  • aste-join: Join a partitioned mesh
  • aste-run: Run ASTE

The current tool names are somewhat not clear are they belong to ASTE tool chain or not.

For instance the calculator tool called vtk_calculator.py as a legacy since the former versions of ASTE was written around a custom file format, as it generalized to VTK file format the tool was extended to have more capability but name was kept.

The scripts partition/join_mesh.py, even the scripts names are clear when they are installed there no sign that shows those scripts are belonged to ASTE.

The main executable preciceMap is somewhat not clear. Now with extended capabilities it is not only using for mapping one data from one mesh to another also it can imitate a solver and can be used for replaying the saved coupling scenarios.

@davidscn

@MakisH
Copy link
Member

MakisH commented Aug 10, 2022

Suggested changes are :

* aste-evaluate: Evaluate a function on a mesh
* aste-partition: Partition a mesh
* aste-join: Join a partitioned mesh 
* aste-run: Run ASTE

One additional thought: if these binaries get installed on a system, the user cannot really relate them to preCICE. So, what if we make them precice-aste-evaluate etc? This then fits nicely with precice-tools and reads a bit like precice-aste --evaluate.

@davidscn
Copy link
Member Author

One additional thought: if these binaries get installed on a system, the user cannot really relate them to preCICE. So, what if we make them precice-aste-evaluate etc?

Sounds reasonable. Then let's go with

  • precice-aste-evaluate: Evaluate a function on a mesh
  • precice-aste-partition: Partition a mesh
  • precice-aste-join: Join a partitioned mesh
  • precice-aste-run: Run ASTE

@fsimonis
Copy link
Member

One additional thought: if these binaries get installed on a system, the user cannot really relate them to preCICE.

I would search for names starting with aste as the installed tool/repo is called aste, not precice-aste.

That said, precice-aste could be a good choice if we ever want to merge aste into precice.

@MakisH
Copy link
Member

MakisH commented Aug 12, 2022

I would search for names starting with aste as the installed tool/repo is called aste, not precice-aste.

But people know and are interested in preCICE, and ASTE is only targeting preCICE. For me, it sounds similar to code_aster providing astk and as_run and a few other tools (which you can install all together or separately). Looking at my system, I have all these binaries that come from the same project, but no clear indication that they are related to code_aster. Similarly for pvserver and paraview. Now imagine my potential confusion since aste and precice-tools (or precice-<anything>) don't even start with the same letter.

We are the developers and we are very familiar with ASTE and know when we need it. But this is not the case for a user that at some point got ASTE installed for some reason (e.g., to run a tutorial).

That said, precice-aste could be a good choice if we ever want to merge aste into precice.

Indeed, this is mostly a problem for that kind of situation, but I still see the precice-aste- prefix as more fitting. For now: low effort/low benefit -> does not really matter.

kursatyurt added a commit to kursatyurt/aste that referenced this issue Aug 15, 2022
@davidscn davidscn linked a pull request Aug 18, 2022 that will close this issue
davidscn added a commit that referenced this issue Aug 18, 2022
* Adapt naming scheme from Discussion: rename executable and python scripts #83

* Change logger names

* Remove extensions from scripts

* Remove extensions from scripts

* Remove obsolete plotstats.py script

* Exit on error in run all script (#132)

* Exit on error in run all script

* Remove redirection

* Run all cases and exit

* Fix syntax; Move sh to bash

* Add missing parantesis

* Update tests/example/run-all.sh

Co-authored-by: David Schneider <[email protected]>

* Update tests/example/run-all.sh

Co-authored-by: David Schneider <[email protected]>

Co-authored-by: David Schneider <[email protected]>

* Remove gradient tags (#130)

Co-authored-by: David Schneider <[email protected]>
davidscn pushed a commit that referenced this issue Sep 18, 2022
* Add documentation CI

* Add documentation

* Add markdown lint options

* Reformat for linter

* Adapt Discussion: rename executable and python scripts #83

* Trailing edge fix

* remove extensions

* Change file names and add step by step guide
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 a pull request may close this issue.

7 participants