Skip to content

Conversation

@AndyBlack
Copy link
Collaborator

@AndyBlack AndyBlack commented Aug 18, 2025

This is a fix related to "LT-22190 Hermit Crab adds wrong POSes to compound rules when there are more than 64 POSes"

The problem is that Machine uses a ulong to represent the possible values of a feature. This solution uses a BitArray instead of a ulong whenever there are more than sizeof(ulong) * 8 (=64) values.

The solution has a subclass of SymbolicFeatureValue called SymbolicFeatureValueBA which is used whenever there are more than 64 possible symbol values. It adds a SymbolicFeatureValueFactory singleton class to create symbolic feature value objects.

The SymbolicFeatureValue class also now has a static method called GetFeatureSymbolFromFeatureStruct(). For reasons not clear to me, the static implicit operator SymbolicFeatureValue(FeatureSymbol symbol) method returns a null when invoked on a SymbolicFeatureValueBA oject. This GetFeatureSymbolFromFeatureStruct() method is a way I found to make things work. If there is a better way to do it, please let me know.


This change is Reviewable

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 64.15094% with 152 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.72%. Comparing base (af3037a) to head (e9efd2b).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
...SIL.Machine/FeatureModel/SymbolicFeatureValueBA.cs 58.46% 87 Missing and 16 partials ⚠️
...c/SIL.Machine/FeatureModel/SymbolicFeatureValue.cs 56.57% 27 Missing and 6 partials ⚠️
...hine.Morphology.HermitCrab/HermitCrabExtensions.cs 80.76% 5 Missing ⚠️
src/SIL.Machine/FeatureModel/SimpleFeatureValue.cs 0.00% 3 Missing and 1 partial ⚠️
...achine/FeatureModel/SymbolicFeatureValueFactory.cs 86.20% 2 Missing and 2 partials ⚠️
src/SIL.Machine/FeatureModel/FeatureStruct.cs 50.00% 0 Missing and 2 partials ⚠️
...achine/FeatureModel/Fluent/FeatureStructBuilder.cs 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #322      +/-   ##
==========================================
- Coverage   70.75%   70.72%   -0.04%     
==========================================
  Files         390      392       +2     
  Lines       32806    33173     +367     
  Branches     4612     4653      +41     
==========================================
+ Hits        23213    23461     +248     
- Misses       8527     8624      +97     
- Partials     1066     1088      +22     

☔ 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.

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ddaspit reviewed all commit messages.
Reviewable status: 0 of 18 files reviewed, 2 unresolved discussions


src/SIL.Machine/FeatureModel/SymbolicFeatureValueFactory.cs line 7 at r1 (raw file):

namespace SIL.Machine.FeatureModel
{
    public class SymbolicFeatureValueFactory

Would it be possible to entirely encapsulate the different implementations of symbolic feature value within the SymbolicFeatureValue class instead of this new factory? You could use composition rather than inheritance to support the implementations. You could define an interface/ABC that abstracts the two implementations. The constructor for SymbolicFeatureValue would create the correct instance and delegate calls to the instance where necessary. This design should greatly reduce the number of changes that are required to the rest of parser.


src/SIL.Machine/FeatureModel/SymbolicFeatureValueBA.cs line 221 at r1 (raw file):

            {
                BitArray notOtherFlags = ((BitArray)otherFlags.Clone()).Not();
                BitArray notOtherFlagsAndFeatureMask = new BitArray(notOtherFlags.And(Feature.MaskBA));

It feels like the number of BitArray instances could be reduced. Could we reuse some of the BitArray instances?

@AndyBlack
Copy link
Collaborator Author

AndyBlack commented Aug 20, 2025

On the BitArray instances: Unlike with ulong, applying any operation to a BitArray changes its value. I ran into failed tests because of one being negated, say. Using Clone() or creating a new instance got around this problem.
Hmm. There are comments about this, so you must mean something else. Please give me an example of what you have in mind.

@AndyBlack
Copy link
Collaborator Author

AndyBlack commented Aug 20, 2025

On using composition: I have no experience with using composition. It sounds good, but I'm afraid I'm rather clueless on how to do it. Could you point me to some examples?
Are you suggesting that there would be three new classes:

  1. Interface of all the methods currently used by both SymboicFeatureValue and SymbolicFeatureValueBA.
  2. A class that implements the ulong approach and this interface
  3. A class that implements the BitArray approach and this interface.

Then the SymbolicFeatureValue class would have two new properties, one for each kind (ulong and BitArray). When creating a new instance of SymbolicFeatureValue, the constructor uses the appropriate property. How then does invoking one of the methods work?

@AndyBlack
Copy link
Collaborator Author

This is replaced by #330

@ddaspit ddaspit deleted the LT22190 branch September 5, 2025 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants