Enhancement/clean up optimisers

[junniest]

Refactored the branch and model optimisation to remove unnecessary RefCell wrappers around cost objects.
Added max_iters to branch length optimisation to speed up long full tree optimisations.

[github-actions]

Test Results

408 tests  +2   407 ✅ +2   18m 4s ⏱️ - 23m 47s
  2 suites ±0     1 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit cee427e. ± Comparison against base commit 785be94.

♻️ This comment has been updated with latest results.

[codecov]

Codecov Report

❌ Patch coverage is 96.96970% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.79%. Comparing base (527040d) to head (cee427e).
⚠️ Report is 10 commits behind head on develop.

Files with missing lines	Patch %	Lines
phylo/src/optimisers/model_optimiser.rs	90.00%	2 Missing ⚠️
phylo/src/optimisers/blen_optimiser.rs	95.45%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #96      +/-   ##
===========================================
+ Coverage    96.32%   96.79%   +0.46%     
===========================================
  Files           32       34       +2     
  Lines         4355     4551     +196     
===========================================
+ Hits          4195     4405     +210     
+ Misses         160      146      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull Request Overview

This PR refactors the tree optimization infrastructure by replacing Vec<bool> dirty tracking with FixedBitSet and removes unnecessary RefCell wrappers around cost objects for cleaner API design.

• Replaces Vec<bool> with FixedBitSet for dirty node tracking in trees
• Removes RefCell wrappers from optimizers and simplifies the TreeSearchCost::update_tree API
• Adds configurable max iterations to branch length optimization for performance

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
phylo/src/tree/tree_parser.rs	Updates tree initialization to use FixedBitSet for dirty tracking
phylo/src/tree/mod.rs	Replaces Vec<bool> dirty field with FixedBitSet and updates related methods
phylo/src/tree/tests.rs	Updates test code to use new FixedBitSet API
phylo/src/substitution_models/mod.rs	Simplifies update_tree method signature and removes dirty nodes parameter
phylo/src/pip_model/mod.rs	Simplifies update_tree method signature and removes dirty nodes parameter
phylo/src/parsimony/cost/tests.rs	Updates test calls to use simplified update_tree API
phylo/src/parsimony/cost/dollo_parsimony_cost.rs	Simplifies update_tree method and removes dirty nodes parameter
phylo/src/parsimony/cost/basic_parsimony_cost.rs	Simplifies update_tree method and removes dirty nodes parameter
phylo/src/optimisers/topo_optimiser.rs	Updates to use simplified update_tree API and adds tree dirtying
phylo/src/optimisers/spr_optimiser.rs	Removes MoveCostInfo dirty nodes tracking and adds max iterations to branch optimizer
phylo/src/optimisers/nni_optimiser.rs	Updates to use FixedBitSet dirty tracking and simplified MoveCostInfo
phylo/src/optimisers/move_optimiser.rs	Removes dirty nodes tracking from MoveCostInfo struct
phylo/src/optimisers/model_optimiser.rs	Removes RefCell wrapper from cost object and adds NaN/negative value handling
phylo/src/optimisers/blen_optimiser.rs	Removes RefCell wrapper, adds max iterations support, and NaN/negative value handling
phylo/src/likelihood/mod.rs	Simplifies TreeSearchCost::update_tree trait method signature
phylo/src/alignment/mod.rs	Fixes logical operator from bitwise OR to logical OR

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[kalxed]

I think this has probably been already thoroughly tested and checked enough but based on what I saw, you moved from vector of booleans to fixed bit set, which is nice change, and reworked how functions work with dirty nodes so that there isn't redundant data being passed around and for the most part it is stored in this bit set for all tree optimisers. Nice work!

[kalxed]

Is this basically so that dirty nodes are not redundantly kept track of within the struct and passed around in functions?

[junniest]

Yes, exactly. The other issue I noticed is that after an SPR move the dirty nodes set in the tree were actually different to the nodes being passed to the update_tree() function. I did not notice that before, but the nodes that were passed weren't correct, it was the regraft and the prune branch, whereas after an SPR move the prune sibling and the new prune parent were the ones that needed to be cleaned up. They could be recalculated, but it seemed unnecessary to do twice, since the dirty nodes are set correctly during the move. With the way it's implemented now there's only one source of information for this and fewer options for errors.

[MattesMrzik]

Looks good, could you pls add the doc that I mentioned below?

[MattesMrzik]

Do we perhaps instead want to have tree.complete be set by Tree::new()? And perhaps also have the compute_postorder() and compute_preorder() be called also by Tree::new()?

[junniest]

Unfortunately, new() doesn't actually make a complete tree, unless the tree only has a single node. It creates a "template" with the right amounts of memory allocated and that template should be filled out by the tree builder. The post- and pre-order traversals can't be generated until all the nodes are properly added.

I don't like the boolean var complete, and would love to get rid of it, and make sure that the orders are properly created (there's no point in time when the tree is accessible but in this weird incomplete stage). However, at this point I am waiting for Kai to merge his PR so that I don't modify too much and make it difficult for him to merge. Once that is merged I will definitely clean this up more!