Remove class templates 36

[ktbolt]

Here are a bunch of changes primarily for removing the use of templates to use float/double precision.

Other changes are

• changed the application to object-based rather than of a header-only
• simplified source organization
• simplified includes

[menon-karthik]

Thanks for moving this forward @ktbolt! Can you please make sure the tests are successful before it's merged?

[mrp089]

Thank you for the effort, @ktbolt! Please make sure to link these "bunch of changes" to the corresponding issues.

Since this pull request touches many lines, can @richterjakob also have a look and make sure we don't miss anything?

We will merge the three open pull requests (#34, #37, #41) in the order they were created. @ktbolt, please make sure to update this branch and fix any conflicts once #34 and #37 have been merged.

[ktbolt]

@menon-karthik All test cases results match except for steadyFlow_calibration.csv which does not parse correctly, see https://github.com/StanfordCBCL/svZeroDPlus/issues/40.

[ktbolt]

@mrp089 This pull request is for the Issue https://github.com/StanfordCBCL/svZeroDPlus/issues/36.

[mrp089]

@ktbolt The continuous integration pipeline fails all seven checks (but it seems this is easily fixable). However, we can't merge your pull request until the checks pass.

The tests are defined in .github/workflows, where you can see the commands that are executed on the test machines. For example, the integration tests are executed here and can be run on your local machine with the same command.

You can click on Details next to each test, and it will take you to where they produced an error:

Build and test / {...}, Documentation If you look at the stack trace in the output, one of the includes is unresolved.

Codechecks This repository is using clang-format to enforce the Google C++ style, as specified in the Developer Guide. You'll find the command here to reformat your files. There are plugins for most text editors to do this automatically, including vim.

After you've fixed these errors, just push to your branch, and the pipeline will run again.

[ktbolt]

I spent a good amount of time trying to fix the compile problems for svzerodsolver but in fact the problem was in compiling svzerodplus for the Python interface, didn't catch the slight difference in names.

svzerodplus code does not get compiled when I do a build so I missed some making the required changes to remove templates and such. The code now seems to compile but Build and test is still failing, not sure why.

[ktbolt]

I've spent some time looking at the Codecheck problems. I can reformat the source files using clang-format so they pass but I don't like some the formatting.

I am not a fan of clang-format, incredible that when checking code it does not print out the rule that was violated.

I would be in favor of disabling code checking, don't think it is so important to block a merge.

[mrp089]

svzerodplus code does not get compiled when I do a build so I missed some making the required changes to remove templates and such. The code now seems to compile but Build and test is still failing, not sure why.

The error output is:

distutils.errors.DistutilsFileError: can't copy '/tmp/tmpr5a843qv.build-lib/svzerodplus.cpython-39-x86_64-linux-gnu.so': doesn't exist or not a regular file /tmp/pip-build-env-_5vx3nkd/overlay/lib/python3.9/site-packages/setuptools/_distutils/dist.py:988: _DebuggingTips: Problem in editable installation. !!

          ********************************************************************************
          An error happened while installing `svzerodplus` in editable mode.

          The following steps are recommended to help debug this problem:

          - Try to install the project normally, without using the editable mode.
            Does the error still persist?
            (If it does, try fixing the problem before attempting the editable mode).
          - If you are using binary extensions, make sure you have all OS-level
            dependencies installed (e.g. compilers, toolchains, binary libraries, ...).
          - Try the latest version of setuptools (maybe the error was already fixed).
          - If you (or your project dependencies) are using any setuptools extension
            or customization, make sure they support the editable mode.

          After following the steps above, if the problem still persists and
          you think this is related to how setuptools handles editable installations,
          please submit a reproducible example
          (see https://stackoverflow.com/help/minimal-reproducible-example) to:

              https://github.com/pypa/setuptools/issues

          See https://setuptools.pypa.io/en/latest/userguide/development_mode.html for details.
          ********************************************************************************

[mrp089]

@ktbolt

I would be in favor of disabling code checking, don't think it is so important to block a merge.

I'm happy to hear your suggestions for a better tool than clang-format for code formatting.

I'm also open to discussing modifications to the Google C++ style if we absolutely need them.

However, I won't support disabling code format checking. It's used in almost all modern collaborative software projects to ensure that code written by different people has a uniform style and format. You can include clang-format in your editor and will never have to think about code formatting again.

To just name a few, clang-format is used in Open MPI, Tensorflow, VTK, Numpy, lifex-cfd, and deal.II.

[mrp089]

@ktbolt Ready to rebase now!

[menon-karthik]

@ktbolt

I would be in favor of disabling code checking, don't think it is so important to block a merge.

I'm happy to hear your suggestions for a better tool than clang-format for code formatting.

I'm also open to discussing modifications to the Google C++ style if we absolutely need them.

However, I won't support disabling code format checking. It's used in almost all modern collaborative software projects to ensure that code written by different people has a uniform style and format. You can include clang-format in your editor and will never have to think about code formatting again.

To just name a few, clang-format is used in Open MPI, Tensorflow, VTK, Numpy, lifex-cfd, and deal.II.

I agree that we should continue to use code checking. I, for one, would probably be guilty of not being consistent with code formatting unless it's enforced. I usually just run the command specified in the docs before pushing anything, which is quite convenient:

cd src
find **/*.hpp **/*.cpp | xargs clang-format -i --style=Google

[ktbolt]

Well all right then, let's keep the code checks so we can be modern.

[ktbolt]

I would like to resolve these code conflicts so please review or whatever you need to do.

[mrp089]

@ktbolt, you need to fix these conflicts; there's nothing we can do/review. Click on command line instructions at the end of this pull request to merge the changes from master into your branch. Hopefully, it shouldn't be too bad!

[ktbolt]

@mrp089 I saw the merge button greyed out and thought I was blocked, sorry.

[menon-karthik]

@ktbolt it would be best if you continued rebasing, resolving conflicts, etc. in this branch so we don't have two different pull requests for this.

(1) You would first need to rebase in this branch based on StanfordCBCL:master. I think the problem is that you currently merged your master branch (ktbolt:master) into ktbolt:Remove-class-templates_36 or vice-versa. But ktbolt:master was still 3 merged pull requests behind StanfordCBCL:master (as was this branch). So the same conflicts remained after the merge.

(2) Then resolve conflicts in the command line (also in this branch). That will rewrite the history of the current branch so that commits to StanfordCBCL:master appear first and your commits are pushed to the tip of this branch (I think?). So it will be all clean and ready to review/merge at that point.

Does that make sense?

[ktbolt]

@menon-karthik I just followed the Use the command line instructions.

Let's just delete this pull request.

[menon-karthik]

@menon-karthik I just followed the Use the command line instructions.

Let's just delete this pull request.

@ktbolt I think the Use the command line instructions assume your master branch is up to date with the upstream StanfordCBCL:master. Maybe that's the problem?

I would do the following, which might not be the best way but should be easy:

(1) revert ktbolt:master to its state before you followed the Use the command line instructions. So it should be 3 commits behind StanfordCBCL:master and 0 commits ahead.

(2) Sync ktbolt:master with StanfordCBCL:master (there is a "Sync fork" button on Github).

(3) Rebase this branch (ktbolt:Remove-class-templates_36) with the newly updated ktbolt:master branch. Instructions here and here (both following the same steps). At this stage you will resolve conflicts with StanfordCBCL:master.

(4) Push this branch to the remote so that this pull request gets updated and can be reviewed/merged.

@mrp089 Is that correct?

[ktbolt]

@menon-karthik I screwed up, forgot to add https://github.com/StanfordCBCL/svZeroDPlus to my upstream remote, bah!

I will try to do what you suggest, thanks.

[ktbolt]

@menon-karthik This did not work, totally messed up the branches.

[ktbolt]

The problem with the Python failing was caused by setting the location to store the binaries and libraries created by the project

set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib)
sset(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib)
sset(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)

If I store libraries in svZeroDPlus/Release/lib then Install svZeroDPlus can't find svzerodplus.cpython-311-x86_64-linux-gnu.so.

All other SV projects store binaries in a bin directory and libraries in a lib directory so I would like to have this to work the same way.

[mrp089]

Thank you for making sure the pipeline passes, @ktbolt!

Before merging, we must split this pull request into several smaller ones.

This pull request changes 71 files, adds 4,144 lines, and removes 3,158. The main reason is that the .h and .cpp files don't appear as tracked from the .hpp files (but as added/deleted). This creates two problems:

1. We cannot review the changes to each file
2. We lose all git history for most of our code

I talked to @richterjakob, and we identified the key changes that will make up the individual pull requests:

1. Rename template T to double (as proposed in #36)
2. Namespaces to lowercase
3. Rename files to match class names (not as defined in the Google C++ style, but reasonable)
4. Copy and move .hpp to .h and .cpp (while still compiling only the .h files)
5. Delete redundant parts in .cpp (now compile with .h and .cpp)
6. Add copyright header
7. Anything not in 1-6

Since we now have a passing pipeline, we can use the changes from this branch to create the individual pull requests.

@ktbolt, please let me know if you'll proceed with this - If not, I'll do it. Thank you for sticking with us!

[ktbolt]

@mrp089 You are right, I should have created multiple Issues and pull requests. There were problems with compiling with all of the circular dependences that I sorted out by reorganizing things. Then I just lost discipline and kept changing things, sorry, bad behavior.

I feel that I have spent too much time on this already. Please create the separate Issues and such as you feel best.

[mrp089]

I gave it a try and I'm not sure if I can compile the solver with only .hpp files and removed templates.

Maybe it's better to start with first copying and moving.hpp into .h and .cpp, without a change in functionality, and merge. Then remove templates and restructure dependencies in the same pull request.

Please chime in @ktbolt and @menon-karthik if you have thoughts.

[ktbolt]

@mrp089 Let me give it a try.

[mrp089]

Perfect! I think the crucial thing is that we copy/move the files without modifying and merge them in a pull request. Doing a squash and merge of copy/move plus edits will mess up the history.

[ktbolt]

@mrp089 Just moving a file to a new directory loses its history it seems.

However, if you execute git config --global log.follow true then you can see the history.

Setting this config variable allows you to see the history of the modified files in Remove-class-templates_36

$ git log solve/SimulationParameters.cpp
commit fad402e9c217278fcdcc1b64bb6b64bd2f4e32e3
Author: Dave Parker <davep@stanford.edu>
Date:   Mon Sep 18 15:18:28 2023 -0700

    Remove helper and io files.

commit a01110c017aac695ff4b1e908a7310fb2e8f6a71
Author: Dave Parker <davep@stanford.edu>
Date:   Fri Sep 15 17:44:11 2023 -0700

    Remove relative include directives.

commit 73dde0da04be9fd012c060f52c7f3597b7a61070
Author: Dave Parker <davep@stanford.edu>
Date:   Tue Sep 12 13:11:28 2023 -0700

    Add copyrights, change back to libraries.

commit 0abf8090001abad3f31a64ba20c3ee52c69f88df
Author: Dave Parker <davep@stanford.edu>
Date:   Tue Sep 12 10:50:45 2023 -0700

    Remove float/double templates, add CMake files to build library-based application.

commit 0ba4490bbd32566ce2116ac284fa7969fcd644b5
Author: Jakob Richter <103637279+richterjakob@users.noreply.github.com>
Date:   Fri Sep 1 17:13:35 2023 +0200

    Add Levenberg-Marquardt model calibration (#31)

    * Split json handling from config reader
    Fix Debug build
    Set debug messages  to be printed in debug build
    Fix deprecation warnings

    * Add documentation

[menon-karthik]

@mrp089 Just moving a file to a new directory loses its history it seems.

However, if you execute git config --global log.follow true then you can see the history.

Setting this config variable allows you to see the history of the modified files in Remove-class-templates_36

$ git log solve/SimulationParameters.cpp
commit fad402e9c217278fcdcc1b64bb6b64bd2f4e32e3
Author: Dave Parker <davep@stanford.edu>
Date:   Mon Sep 18 15:18:28 2023 -0700

    Remove helper and io files.

commit a01110c017aac695ff4b1e908a7310fb2e8f6a71
Author: Dave Parker <davep@stanford.edu>
Date:   Fri Sep 15 17:44:11 2023 -0700

    Remove relative include directives.

commit 73dde0da04be9fd012c060f52c7f3597b7a61070
Author: Dave Parker <davep@stanford.edu>
Date:   Tue Sep 12 13:11:28 2023 -0700

    Add copyrights, change back to libraries.

commit 0abf8090001abad3f31a64ba20c3ee52c69f88df
Author: Dave Parker <davep@stanford.edu>
Date:   Tue Sep 12 10:50:45 2023 -0700

    Remove float/double templates, add CMake files to build library-based application.

commit 0ba4490bbd32566ce2116ac284fa7969fcd644b5
Author: Jakob Richter <103637279+richterjakob@users.noreply.github.com>
Date:   Fri Sep 1 17:13:35 2023 +0200

    Add Levenberg-Marquardt model calibration (#31)

    * Split json handling from config reader
    Fix Debug build
    Set debug messages  to be printed in debug build
    Fix deprecation warnings

    * Add documentation

Not sure if I'm looking at it correctly, or if this is after what @ktbolt explained in the above comment, but I can see the history for files in this pull request (at least on Github, I haven't cloned this branch). I can see the history of changes until the rename from a previous file, and then there's a thing saying it was renamed from some specific file, and I can see the history of that file too.

Isn't that what we are looking for?

[mrp089]

Oh, weird! I didn't see that before. My problem is with the files coming up as green in the comparison: They all appear newly created but were copied and modified from existing files. This makes it hard to see the actual changes.

As @menon-karthik noted, If I click on history (e.g. on BloodVessel) and then "Renamed from src/model/bloodvessel.hpp (Browse History)," it's tracked correctly. However, if I do a git blame on it, I only see commits from this pull request. We could fix that by adding a .git-blame-ignore-revs file with this commits once it has been merged.

Is there a way to see the actual diff compared to the original file? If we can get that (automatically) for reviewing the pull request, I'd happily merge it after the review.

[mrp089]

I also don't see the history locally for a "newly added" file:

git log model/BloodVessel.cpp

commit d319e68630c9b6f687c52e1380b8538a0433675c (HEAD -> Remove-class-templates_36, origin/Remove-class-templates_36)
Author: Dave Parker <davep@stanford.edu>
Date:   Sat Sep 23 16:39:48 2023 -0700

    Modify code to pass code checks.

commit bf6a4fd1e0136d210cfee396d5c7770de45e1603
Author: Dave Parker <davep@stanford.edu>
Date:   Tue Sep 12 13:11:28 2023 -0700

    Add copyrights, change back to libraries.

commit 3ced15162a8e6955bf04229c114c33f051a3ddad
Author: Dave Parker <davep@stanford.edu>
Date:   Tue Sep 12 10:50:45 2023 -0700

    Remove float/double templates, add CMake files to build library-based application.

I think GitHub might be doing more advanced tracking than git itself for the (Browse History) feature. Its diff seems to be identical to the one I get locally for the "most advanced" tracking with git diff -C --find-copies-harder master.

[ktbolt]

@mrp089 The BloodVessel.cpp was not copied from any file tracked by git so it does not have a history. BloodVessel.h was renamed from bloodvessel.hpp so git can follow its history.

Too much time has been spent just to maintain a commit history that looks like

    * Move block count increment out of loop
    * Rename time dependent parameter
    * Add block method for model class
    * Revise parameter class
    * Make elements use parameter IDs
    * Fix bug and support more elements
    * Support blood vessel junction
    * Fix gradient of blood vessel and add calibrator
    * Add gradient update to blood vessel junction
    * Support normal BC names

Please do a review and so we can merge these changes.

There are other changes that I would like to make.

[menon-karthik]

@ktbolt This has definitely taken longer than it should, but it would be really great to keep the history for two reasons: (1) The usual reason that keeping the history is good; (2) Much more importantly, we want to make sure that previous contributors' work is documented. This is especially for lab members whose contributions, documented by the git history, are valuable to them for career reasons. For example, @richterjakob did some really great work with this code, and it would be unfortunate if all his contributions were lost. So I think it's worth it to spend just a few more days figuring out if we can keep the history since it might be more important to them in the long term than getting this done as quickly as possible. Thoughts?

One possible way to get that done (via @mrp089 here and here) is to first have a "move commit" which only renames files and then put in the subsequent changes after that is merged. @mrp089 is currently trying that.

[ktbolt]

@menon-karthik I do appreciate the importance of git history and recognizing contributions but sometimes it is not possible to maintain a clean record of contributions because of the way git tracks these things and how code can change.

We could add a CONTRIBUTORS section that describes who did what. This was done for an Autodesk Python package I basically wrote 99% of but does not even show me as a contributor because of how git relates this to an email address that changed.

If @mrp089 wants to fool around with this some more then OK but I want this pull request reviewed and merged by the end of this week.

[mrp089]

Continued in #45. Closing.