search

waynebhayes / SANA

Simulating Annealing Network Aligner
25 stars 40 forks source link

Refactor CMake to adhere to best practices #96

Closed vietly23 closed 5 years ago

vietly23 commented 5 years ago

Refactoring CMake to solve the following problems: 1) We are polluting the src directory with a bunch of generated files. 2) GTest Suite and mocking library are merged together now in Git. Current build is broken 3) Globbing recursively will can lead to silent successes even if files are removed or deleted - CMake themselves "do not recommend to use GLOB to collect a list of source files from your source tree"

Adding in base directory script to generate distribution in build/bin/ as well as remove brittleness with cppcheck.

Fixed Graph and ExternalWeightedEdgeConservation constructors which were not explicitly setting default values.

Testing Done

Test Output

lyvl@lyvl-VirtualBox:~/Desktop/SANA/build$ make test 
Running tests...                                                                                                                   
Test project /home/lyvl/Desktop/SANA/build
    Start 1: test-lib-sana
1/1 Test #1: test-lib-sana ....................   Passed    0.00 sec  

100% tests passed, 0 tests failed out of 1

Total Test time (real) =   0.00 sec
waynebhayes commented 5 years ago

Viet,

This is a lot of stuff to change, and I'm not entirely sure it was necessary. Did you run all the regression tests that already exist? They're run by running ./regresion-test-all.sh.

Also, Nil Mamano is making a bunch of changes to the temperatue schedule and I don't think he's finished committing his changes yet. Having two people making sweeping changes at the same time before committing isn't a good idea.

Also, from a scientific point of view there are many things that are much higher priority than the Makefile. For example, there are issues to do with the multiple aligner (that's mostly your baby since you're the one who implemented WEIGHTED---now called MULTI_PAIRWISE since we've generalized "weights" now to include real-number weights); for example at least two of the most important objective functions for multiple alignment are not working yet: MS3 and MEC. William had started working on them but didn't finish before graduating.

So, I'm going to hold off on accepting this pull request until Nil has committed his changes on the temperature scheduler, because those are scientifically more important and he's defending his Ph.D. soon and won't have any more time after that. You, on the other hand, seem to pop up every 6 months. :-)

If you have time to do SANA stuff, in the future please consult me first on what's the highest priority. Right now it's (a) ensuring things don't break, and (b) getting the temperature scheduler finalized.

If you're interested, I can send you a recent paper I'm about to submit. It's only on the pairwise aligner but it's the first demonstration ever that pairwise alignment can recover functional similarity.

Keep in touch. :-)

-- Wayne Hayes, Ph.D. Associate Professor of Computer Science, University of California, Irvine Director, UCI-SDSU Joint Ph.D. Program in Computational Science (UCI side) Has anything turned more people away from the power above the heavens than the power below their waists? Wauism doesn't have that problem, because as a Wauist, you can stick or get stuck however you want with whom or whatever you want whenever or wherever you want. As long as no one gets hurt--or only if they want to--Wauism says have fun. And be safe.--Wausim https://www.netfunny.com/rhf/jokes/91q4/wauism.html

On Sun, Aug 18, 2019 at 6:57 PM SpicyMeatball notifications@github.com wrote:

Refactoring CMake to solve the following problems:

  1. We are polluting the src directory with a bunch of generated files.
  2. GTest Suite and mocking library are merged together now in Git. Current build is broken
  3. Globbing recursively will can lead to silent successes even if files are removed or deleted - CMake themselves "do not recommend to use GLOB to collect a list of source files from your source tree"

Adding in base directory script to generate distribution in build/bin/ as well as remove brittleness with cppcheck.

Fixed Graph and ExternalWeightedEdgeConservation constructors which were not explicitly setting default values. Testing Done

  • Verified that the project can build: by running ./make-distribution in the directory, then running make in build/
  • Tests pass, see "Test Output"
  • Updated README to reflect build practices

Test Output

lyvl@lyvl-VirtualBox:~/Desktop/SANA/build$ make test Running tests... Test project /home/lyvl/Desktop/SANA/build Start 1: test-lib-sana 1/1 Test #1: test-lib-sana .................... Passed 0.00 sec

100% tests passed, 0 tests failed out of 1

Total Test time (real) = 0.00 sec

You can view, comment on, or merge this pull request online at:

https://github.com/waynebhayes/SANA/pull/96 Commit Summary

  • Refactor CMake to adhere to best practices

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/waynebhayes/SANA/pull/96?email_source=notifications&email_token=AE2SJDHH7QSM7FJAKHSPGBDQFH4XXA5CNFSM4IMVF3SKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HF3ZYFQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AE2SJDH2LOH6FJX3PFTAJHDQFH4XXANCNFSM4IMVF3SA .

vietly23 commented 5 years ago

Hi Professor Hayes,

There are no code modifications, and the ./regression-test-all.sh is not present in the development branch, so I could not run the script.

I agree that the scientific priorities are much higher than altering some Makefile (or CMake file in this case), but the engineering is much more important. From the engineering standpoint, we need to make sure the infrastructure is in place before adding new features. To make multi-pairwise work on a scale and on a distributed basis, we need to refactor.

For instance, I see that we recently added support for loading Graphs from serialized binary representations. However, the third-party library was added directly to the source code. What happens if we need to add multi-threading? Communication across distributed systems? Or use several third-party libraries to reduce coding churn and increase readability?

It's also unclear how we are linking the GTest suite: https://github.com/waynebhayes/SANA/search?q=gtest&unscoped_q=gtest. Are we downloading the library somewhere else, building, then moving the library to the test directory to link statically?

I understand that Nil is working on his thesis, which is why I'm making this pull request to the development branch, not SANA-1.1.

waynebhayes commented 5 years ago

Aha. I failed to notice this pull request was on the development branch... so, I'm happy to accept it now. However.... SANA1.1 is where all the action is now. A lot of features have been added/changed, and I think the development branch is probably diverging further and further from SANA1.1.

Maybe you should come back for a Ph.D. :-)

-- Wayne Hayes, Ph.D. Associate Professor of Computer Science, University of California, Irvine Director, UCI-SDSU Joint Ph.D. Program in Computational Science (UCI side) Has anything turned more people away from the power above the heavens than the power below their waists? Wauism doesn't have that problem, because as a Wauist, you can stick or get stuck however you want with whom or whatever you want whenever or wherever you want. As long as no one gets hurt--or only if they want to--Wauism says have fun. And be safe.--Wausim https://www.netfunny.com/rhf/jokes/91q4/wauism.html

On Sun, Aug 18, 2019 at 8:12 PM SpicyMeatball notifications@github.com wrote:

Hi Professor Hayes,

There are no code modifications, and the ./regression-test-all.sh is not present in the development branch, so I could not run the script.

I agree that the scientific priorities are much higher than altering some Makefile (or CMake file in this case), but the engineering is much more important. From the engineering standpoint, we need to make sure the infrastructure is in place before adding new features. To make multi-pairwise work on a scale and on a distributed basis, we need to refactor.

For instance, I see that we recently added support for loading Graphs from serialized binary representations. However, the third-party library was added directly to the source code. What happens if we need to add multi-threading? Communication across distributed systems? Or use several third-party libraries to reduce coding churn and increase readability?

It's also unclear how we are linking the GTest suite: https://github.com/waynebhayes/SANA/search?q=gtest&unscoped_q=gtest. Are we downloading the library somewhere else, building, then moving the library to the test directory to link statically?

I understand that Nil is working on his thesis, which is why I'm making this pull request to the development branch, not SANA-1.1.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/waynebhayes/SANA/pull/96?email_source=notifications&email_token=AE2SJDAW4THHURXNEO26LBLQFIFRRA5CNFSM4IMVF3SKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4RR7SQ#issuecomment-522395594, or mute the thread https://github.com/notifications/unsubscribe-auth/AE2SJDGWFQVWRBLLQW4JL2DQFIFRRANCNFSM4IMVF3SA .