Hi authors! I have a few comments regarding to the review.

[x] The package NetworkX should be added as a required package
[x] Can you provide a case similar to the extra material at https://www.sciencedirect.com/science/article/pii/S0098135424000681?via%3Dihub that calculates RTD? In other words, the script that reproduces Figure 4 in the JOSS paper. I'm especially interested in seeing a case based on OpenFOAM flow results.
[x] Minor edit: the code needs Administrator privilege to run on Windows due to os.symlink. It might be good to throw an error if the requirement is not met at the beginning.

NetworkX The package is currently listed as an optional requirement as it isn't needed for the main functionality of the package. It's only needed for certain optional post processing functionality, specifically if network_diagram is set to True, which is the current default.

[x] Do you still get a package not found error if network_diagram is set to False in the config file? (It goes under POST-PROCESSING). If so, this is an oversight on my part as the imports were supposed to be wrapped so that they would only be called when that flag was used.
[x] Would changing the provided examples, either by overriding network_diagram in their config file or by changing the default value, such that they do not use the NetworkX code path be an acceptable solution?

RTD I will look into incorporating that calculation script into the OpenCMP example provided, as that example and the simulation from the paper are derived from the same source.

Symlink I will take a look and test alternatives for symlinking. I'm thinking automatically falling back to a plain file copy, and displaying a warning, would be preferable to throwing the error.

This would let the user know that a faster alternative is available, but does not force them to run with admins privileges, which may be an impossible expectation in certain corporate engineering settings. And it would generally be less risky for the user to not have to run it with admin privileges.

@luohancfd Hi there, let me know if the changes in #38 properly address your above comments.

@Alex-Vasile

I'm struggling with the example cases and need your help to address them.

examples/OpenCMP: I assume config_Stokes generates and initial guess, config_INS is for single specie transient incompressible flow simulation, config_MCINS is for multi-species one. Although the usage of OpenCMP is beyond the topic of this repo, it's better to document that. I ran the config_Stokes case successfully. But the config_INS case took extensive time. Is it really necessary to run to 5000s? The same reason applies to the config_MCINS. I found I can still run the compartment modeling part with the time_range reduced to 30s and get similar plot by running analysis.py. If it's really necessary, please provide an example output. I found you have results published https://www.sciencedirect.com/science/article/pii/S0098135424000681?via%3Dihub for config_INS but nothing for config_MCINS. Besides, I'm a little confused why you can compare the compartment modeling with the config_MCINS. In the compartment modeling, it seems you are modeling an almost irrevisble reaction Na2CO3 + CaCl2 -> 2NaCl + CaCO3 (I assume this is gas phase reaction). But I couldn't understand what the case config_MCINS is doing. According to this tutorial case https://opencmp.io/tutorials/tutorial_8.html, you are solving an additional mass conservation equation as a. But I couldn't find a similar [SURFACE_RXN] block. You do have a block named [FUNCTIONS]/source in file model_config, but it's 0 for a. I personally feel it's OK to not explain every details of a case, especially when the case is targeted for a specific academic usage. But if you decide to publish your code, it's better to let the user understand what is going on for the example cases.
examples/OpenFOAM: I built OpenFOAM-12 download link https://openfoam.org/download/12-source/, sha256 are listed here in case it matters : e59fad54c62e64f1bb89dbaebe5f99a76dc0a6a91d9aad86042a7c4cef6d0744 OpenFOAM-12.tar.gz 8159763451f714387a3846e9c09717533f293107204a3ad470e4acbaa8ce356b ThirdParty-12.tar.gz. I didn't change the mesh. So I just renamed folder "_0" to "0", and invoke foamRun (step 5 in your instruction). I didn't see any difference of T and Vc for 0 and 300. Is that the expected behavior? In you instruction, you mentioned
1. Open controlDict, comment out the solve line with incompressibleFluid and uncomment the two lines below it defining a new solve and subSulver, finally uncomment the #includeFunc scalarTransport line at the bottom of the file.

But the file you posted https://github.com/uw-comphys/openccm/blob/a18be55ddb1fb0f0f501c3f7d38c4b68cab9b51f/examples/OpenFOAM/pipe_with_recirc/system/controlDict#L19-L24

has already uncommented the lines.

If this is a mistake, please fix it. Also, please provide a tagged frozen version for review. While I'm writting this comment, I see a new PR get merged.

pytests: I failed to run the tests due to missing files from the examples folder. Please correct the examples, or provide independent test files that don't rely on other data/folder.

@luohancfd

I looked into it a bit more, and I'll include some of the pre-computed files. To clarify some of your other points.
- Yes, the Stokes solve is used as an initial condition for the INS solve.
- No, INS is for only the hydrodynamics with no species transport. MCINS is for n-component reactive flows coupled with INS for hydrodynamics. Here Stokes and INS are used to find the steady state velocity profile in this domain, which one exists. That steady-state velocity profile is then fixed, to speed up the computation, and used by MCINS to calculate the species transport.
- The time range for Stokes and INS were leftover from when those simulations were transient, the program solves directly for the steady-state flow profile. The value for the time was irrelevant since it wasn't used. I have removed it as it may confuse others. The time it takes to solve the INS simulation is dictated by the size of the resulting matrix and how many non-linear iterations are required to obtain the steady-state.
- There are two compartmental model configuration files inside pipe_recird_2d:
  1. CONFIG: Creates a compartmental model and then runs an inert tracer simualtion on it for calculating residence time. This is the same as the CFD simulation described by CONFIG_MCINS. This simulation is what gets compared to the CFD results.
  2. CONFIG_W_RXN: This also creates a compartmental model but it shows how to run a reactive species simulation, using a reversible reaction and chemical specie variable names. This is simulation is not compared to any CFD.
The 0/ directory contains the pre-computed steady-state results. The instructions state that if you wish to use these results and go straight to running the inert tracer CFD simulation to skip to step 13. I have changed the wording to make this more clear.
The tests should have all of the data they need, I updated them in another PR to generate any input data they need. However, They need to be run from inside the pytest folder. ~~#43~~ #50 addresses this issue. Once that's merged the tests can be run using the openccm-tests entry point from any directory.

See #49 for changed related to these comments.

50 addressed the test running, they can now be run using the `openccm-tests` entry point.

@Alex-Vasile

I can run pytest successfully but still have issue with the OpenFOAM test. I followed the README.md file and directly run foamRun. Here is the result I got for T @t=300

When I run the analysis.py, I got the following

(venv) han:/home/han/Documents/JOSS/openccm/examples/OpenFOAM/pipe_with_recirc/ (main✗) $ python analysis.py
Using the default value of 8 for SETUP, num_cores.
Using the default value of False for SETUP, DEBUG.
Using the default value of ./ for SETUP, working_directory.
Using the default value of output_ccm/ for SETUP, output_folder_path.
Using the default value of cache/ for SETUP, tmp_folder_path.
Using the default value of log/ for SETUP, log_folder_path.
Using the default value of 1e-15 for COMPARTMENT MODELLING, flow_threshold.
Using the default value of 1e-15 for COMPARTMENT MODELLING, flow_threshold_facet.
Using the default value of 0.05 for COMPARTMENT MODELLING, dist_threshold.
Using the default value of 0 for COMPARTMENT MODELLING, rtol_opt.
Using the default value of 0.01 for COMPARTMENT MODELLING, atol_opt.
Using the default value of 0.0001 for SIMULATION, first_timestep.
Using the default value of LSODA for SIMULATION, solver.
Using the default value of None for SIMULATION, reactions_file_path.
Using the default value of 1e-06 for SIMULATION, rtol.
Using the default value of 1e-06 for SIMULATION, atol.
Using the default value of True for POST-PROCESSING, save_to_file.
Using the default value of True for POST-PROCESSING, network_diagram.
Using the default value of 0 for POST-PROCESSING, subdivisions.
Using the default value of 1 for POST-PROCESSING, interpolant_order.
Using the default value of compartment_pfr_vtu/ for POST-PROCESSING, vtu_dir.
Calculating compartments
Traceback (most recent call last):
  File "/home/han/Documents/JOSS/openccm/examples/OpenFOAM/pipe_with_recirc/analysis.py", line 194, in <module>
    data_pfr, data_cstr  = rtd_cm()
                           ^^^^^^^^
  File "/home/han/Documents/JOSS/openccm/examples/OpenFOAM/pipe_with_recirc/analysis.py", line 149, in rtd_cm
    run(config_parser)
  File "/home/han/Documents/JOSS/venv/lib/python3.11/site-packages/openccm/run.py", line 111, in run
    compartments, _ = calculate_compartments(dir_vec, c_mesh, config_parser)
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/han/Documents/JOSS/venv/lib/python3.11/site-packages/openccm/compartmentalize/unidirectional.py", line 286, in calculate_compartments
    compartment_of_removed_elements, valid_elements = _remove_unwanted_elements(mesh, dir_vec)
                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/han/Documents/JOSS/venv/lib/python3.11/site-packages/openccm/compartmentalize/unidirectional.py", line 968, in _remove_unwanted_elements
    for element_neighbour in mesh.element_connectivity.pop(element):
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'tuple' object has no attribute 'pop'

Here is a list of pip packages

Package            Version
------------------ ---------------
configparser       7.1.0
contourpy          1.3.0
cycler             0.12.1
fonttools          4.53.1
iniconfig          2.0.0
intel-cmplr-lib-ur 2024.2.1
intel-openmp       2024.2.1
kiwisolver         1.4.7
llvmlite           0.43.0
matplotlib         3.9.2
mkl                2024.2.2
mpmath             1.3.0
netgen-mesher      6.2.2404.post16
netgen_occt        7.8.1
ngsolve            6.2.2404.post51
numba              0.60.0
numpy              2.0.2
openccm            0.1
opencmp            1.0.2
packaging          24.1
pillow             10.4.0
pip                24.0
pluggy             1.5.0
pyparsing          3.1.4
pytest             8.3.3
python-dateutil    2.9.0.post0
scipy              1.14.1
setuptools         65.5.0
six                1.16.0
sympy              1.13.3
tbb                2021.13.1

@luohancfd Sorry about the late reply.

The CFD results look correct. That semi circular region requires a very long time to fully saturate. The provided example is capped to t=300s to greatly speed up the time taken to perform the analysis. One could extend the simulation time to several thousand seconds, but the analysis results are not substantially changed.

As for the error you're seeing. I believe this is caused by a mixture of a new version of the analysis script (which you run from JOSS/openccm/) and an outdated version of openccm which is called from JOSS/venv/lib/python3.11. The code that is raising the error was changed in #30 (though the error itself appears to be caused by a mix of old code but a new cmesh version).

Please remove the old version from pip, pull the newest version from GitHub, and then pip install. Let me know if the issue persists afterwards.

uw-comphys / openccm

JOSS review #37

50 addressed the test running, they can now be run using the `openccm-tests` entry point.

uw-comphys / openccm

JOSS review #37

50 addressed the test running, they can now be run using the openccm-tests entry point.

50 addressed the test running, they can now be run using the `openccm-tests` entry point.