ENH/BUG: Code Clean Up, Optimization, Documentation

[TactfulDeity]

Request:
I would like to request a squash commit if this goes in, I ran the clang formatting with style file, but it doesn't pick up all of it that actions bot does hence the needless third commit.

Preface:
I only did the calc, core, and io folders completely, but I touched on other smaller plugins as well. I am not overly familiar with QT so I was cautious about removing those headers. I only did what I could finish over the long weekend, but I am willing to do more down the line. Special care should be taken reviewing qtaim plugin, I consolidated and cleaned up a lot, so I want to make sure I didn't break it.

Fixes:
#1776

List of Changes:

• Concatenated namespaces
• removed unneeded includes
• changed bool declarations and if checks to use lazy evaluation and cut down on needless branching
• changed places where ADL was not neccessary and added documentation and scoping to places that may benefit
• extensive changes to qtaim plugin (should be reviewed with scrutiny)
• removed using std::namespace
• changed typedef to using statements
• changed codes to remove obvious excessive copies
• fixed auto declarations to include pointer/ref
• extensive changes to default constructors/destructors
• documentation in the form of comments where appropriate
• small changes to prefer c++ standard structures where appropriate

Note:
I went through this in order to familiarize myself with the code base better, during which I noticed there are a bunch of "utility" classes that basically act as an interface of sorts. I think these could be changed to instead be namespaces to remove unneeded object overhead. Example: atomutilities.h. I will add these to the list of changes I can help with if other maintainers agree.

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

[TactfulDeity]

I suspect it will need some include fixes for each of the platforms before going in, I will finish patching tomorrow and mark it ready for review after I get it passing.

[ghutchis]

I can definitely do a squash commit, but I've also had a few that I should have squashed in the commit history.

I'll take a look tomorrow, thanks.

[ghutchis]

BTW, GitHub is a little weird - you have to have "Fixes #1776" on one line for it to recognize the issue. I manually marked it though.

[github-actions]

Here are the build results
Win64.exe
Artifacts will only be retained for 90 days.

[avo-bot]

This pull request has been mentioned on Avogadro Discussion. There might be relevant details there:

https://discuss.avogadro.cc/t/november-2024-live-updates/6446/1

[TactfulDeity]

Looks ready to go it passed ARM I will check back in and make any changes if another issue crops up in the bots.

[github-actions]

Here are the build results
Avogadro2.AppImage
Ubuntu-Latest.tar.gz
macOS.dmg
Win64.exe
Artifacts will only be retained for 90 days.

[ghutchis]

Is this ready for review? If not, @ me when you think it's ready to examine. Thanks - definitely a big help.

[TactfulDeity]

Not yet I am going to squash the last two bugs picked up by the address sanitizer and undefined behavior sanitizer as soon as I get out of work today

[TactfulDeity]

I will definitely '@' you as soon as I get it working. Sorry its taking so long, I don't have a lot of time during the week.

[ghutchis]

Oh, no worries. This isn't taking "long." And all of us are doing this in our "free" time. 😄

Much appreciated whenever you get to it.

[github-actions]

Here are the build results
Ubuntu-Latest.tar.gz
Avogadro2.AppImage
macOS.dmg
Win64.exe
Artifacts will only be retained for 90 days.

[ghutchis]

Let me finish the review - a few other minor notes... but overall, it looks good.

[TactfulDeity]

@ghutchis I actually did have a question about the code. I am working on trying to fix whatever bug I introduced to graph.cpp.

In the Ubuntu address sanitizer run you can see it's actually failing a test case "connected components" (line 86 of Run tests).

However, I don't actually know if the result is wrong or the test case is wrong. I drew out a graph for the test case and it starts like this:

[0] [1] [2] [3] [4] [5]

Where each bracketed number is a node/vertex.

It ends (right before the assert after the final edge removal) looking like this:

[0]-[1]-[2] [3] [4]-[5]

Where each "-" represents an edge.

For reference the test case is on line 146 of graphtest.cpp.

The assert asks for the connected components count and says it should be "4". I am confused about where the 4 is coming from. I think it may be due to some part of how the implementation handles lone nodes or separate subgraphs? Any insight you may have would be greatly appreciated. I feel if I can understand the intent I can identify the bug better.

I poured over my changes to graph (and the member functions in general) a couple times yesterday but couldn't figure out what would cause this. I reverted the ADL on swap thinking maybe it was calling an unintended method that wasn't doing what was expected, but the issue persisted in the run from my most recent commit.

Thanks for all your time and consideration in helping me get spun up on the project.

[ghutchis]

A few minor things...

[TactfulDeity]

@ghutchis okay, I am out of work and actively working on making review changes. Did you see my message higher up about the graph bug?

[ghutchis]

@ghutchis I actually did have a question about the code. I am working on trying to fix whatever bug I introduced to graph.cpp.

Not sure I'll have a chance to look tonight, but I'll take a look tomorrow.

[github-actions]

Here are the build results
Avogadro2.AppImage
Ubuntu-Latest.tar.gz
Win64.exe
Artifacts will only be retained for 90 days.

[TactfulDeity]

Alright that concludes the review related patches, none of which touched the graph file, so the test is likely still failing for some reason beyond me. The sanitizer and undefined behavior were skipped for some reason so I am caching the error from previous commit here:

11/37 Test #11: Core-Graph .......................***Failed    0.05 sec
Running main() from /home/runner/work/avogadrolibs/build/thirdparty/gtest-prefix/src/gtest/googletest/src/gtest_main.cc
Note: Google Test filter = GraphTest.*
[==========] Running 11 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 11 tests from GraphTest
[ RUN      ] GraphTest.size
[       OK ] GraphTest.size (0 ms)
[ RUN      ] GraphTest.setSize
[       OK ] GraphTest.setSize (0 ms)
[ RUN      ] GraphTest.isEmpty
[       OK ] GraphTest.isEmpty (0 ms)
[ RUN      ] GraphTest.clear
[       OK ] GraphTest.clear (0 ms)
[ RUN      ] GraphTest.addVertex
[       OK ] GraphTest.addVertex (0 ms)
[ RUN      ] GraphTest.removeVertex
[       OK ] GraphTest.removeVertex (0 ms)
[ RUN      ] GraphTest.vertexCount
[       OK ] GraphTest.vertexCount (0 ms)
[ RUN      ] GraphTest.addEdge
[       OK ] GraphTest.addEdge (0 ms)
[ RUN      ] GraphTest.removeEdge
[       OK ] GraphTest.removeEdge (0 ms)
[ RUN      ] GraphTest.edgeCount
[       OK ] GraphTest.edgeCount (0 ms)
[ RUN      ] GraphTest.connectedComponents
/home/runner/work/avogadrolibs/avogadrolibs/avogadrolibs/tests/core/graphtest.cpp:169: Failure
Expected equality of these values:
  graph.connectedComponents().size()
    Which is: 5
  static_cast<size_t>(4)
    Which is: 4
[  FAILED  ] GraphTest.connectedComponents (0 ms)
[----------] 11 tests from GraphTest (1 ms total)

[----------] Global test environment tear-down
[==========] 11 tests from 1 test suite ran. (1 ms total)
[  PASSED  ] 10 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] GraphTest.connectedComponents

 1 FAILED TEST

[ghutchis]

I had to have a moment to step through the test again. Here's what it should look like at the end of the test:

(i.e., it removed all edges around node 4)

So yeah, it's a little weird that it's now reporting 5.

I also don't see anything immediately obvious in what you've done. I'll look through more carefully.

[ghutchis]

Otherwise, the PR looks good. Fix the weird iterator and we're good to merge.

[TactfulDeity]

@ghutchis Sweet, thanks so much for all your help. I'll get a fix in in a few hours before I head to bed tonight, so it should be ready to go (checks passed) in the AM.

[TactfulDeity]

Alright fix is in and ready to go

[github-actions]

Here are the build results
Avogadro2.AppImage
Ubuntu-Latest.tar.gz
macOS.dmg
Win64.exe
Artifacts will only be retained for 90 days.

[github-actions]

Here are the build results
Ubuntu-Latest.tar.gz
Avogadro2.AppImage
Win64.exe
Artifacts will only be retained for 90 days.