Fix MutSel potential bugs related to alignment copying

[StefanFlaumberg]

This PR simplifies and solidifies the logic of how pattern-specific rate matrices are handled. It introduces the following changes:

• Store pattern-specific rate matrices of an alignment in the vector<double*> Alignment::ptn_rate_mat vector as C-style arrays in the row-major order. This design makes ptn_rate_mat largely similar in use to ptn_state_freq.
• Add the int Alignment::num_rates member to make ptn_rate_mat compatible with various data types and non-reversible models and possibly useful beyond MutSel.
• Ensure ptn_rate_mat are copied along with ptn_state_freq by the alignment-copying functions (extractSubAlignment(), extractPatterns(), extractSites(), etc.). IMPORTANT: This fixes potential bugs of MutSel related to those I previously fixed for PMSF.
• Minor changes in mutsel_wrapper.cpp: rate matrix variables use names and data types more similar to those of frequency vector variables; the write_site_models_to_alignment() function prints at the end the total number of the pattern-specific matrix-vector pairs instead of site-specific such pairs, which is now similar to the Alignment::readSiteStateFreq() function's printed log.

[bqminh]

mostly good thank you. I'll merge this now and update one thing to remote num_rates from Alignment

[benjamin-lieser]

Do you think it would be better to change it from a pointer to either a std::vector or std::unique_ptr? This should be a small change.

I feel otherwise this will inevitably (or already does) lead to memory leaks and use after free.

[StefanFlaumberg]

@benjamin-lieser,

Your suggestion for using vectors and smart pointers is reasonable from the standpoint of the modern C++ standards, and I would definitely opt to follow these standards myself if I were to write a new program. But here we are discussing a small addition to the IQ-Tree code, in which there are C-style arrays everywhere and hardly any smart pointers at all. So in order to be consistent, we should either decide on rewriting the whole program or just opt to follow the existing style. It is worth noting that using C-style arrays is not evil by itself, but merely has to be done with a bit of caution.

Why there is definitely no usage after delete now:
Because your test example (now removed by Minh, though) had run fine on this PR.

Why there are no memory leaks and any such bugs are very unlikely to appear in the future:
Because the code logic prevents them. After being put into ptn_rate_mat or ptn_state_freq the array pointers are kept there until being deleted by the ~Alignment() destructor. Since the regroupSitePattern() function can split the existing patterns (but never merge them), some of the pointers can be assigned to multiple patterns, but the destructor covers this as well. Various alignment-copying functions use these parameters, but they just add new arrays into new alignments and copy the existing values with memcpy therein, so no problems here. ModelSet also just copies these parameters.
So everything is just deleted by the destructor and there seems to be no conceivable way to bypass it.

The occasional need to assign a single array pointer to multiple patterns, as it is done in the regroupSitePattern() function, is exactly the reason why neither vectors nor unique_ptrs can be used here. One, however, could use shared_ptrs I guess. Then both ptn_rate_mat or ptn_state_freq would have the type of std::vector<std::shared_ptr<std::vector<double>>>. Looks quite complex, but keeps all the functionality and has a clear advantage of directly providing the size of a rate matrix via std::vector<double>::size() without the need of using a separate member variable or function.

I suggest a compromise solution. We'll keep the types of both members as std::vector<double *> for now, since it's better to keep them similar and my PMSFR model PR code is already based on ptn_state_freq having that type. But after merging that huge PR, I'll see if it is possible to switch to the shared_ptr solution discussed above.

[bqminh]

Agreed! Actually there are many things that I could have done differently if we were to write IQ-TREE now. The original code was designed 15 years ago, and over time I found things that could have been better designed. For example, the tree data structure can be better designed (e.g. merging Node and Neighbor). The Alignment structure can also be embedded better into the likelihood calculations (as Alignment sits at the tips of the tree), so that likelihood is computed more efficiently. In any case, one thing we need to keep is: to retain the full git history, so that contributions of all developers/contributors are clearly recognised.

When I have time I will write down all the ideas I may have wanted to do differently. I thought it can never come to fruition, but now with the age of AI, it may become possible, actually.