iho-ohi / S-101-Documentation-and-FC

Repository issues of S-101 document and feature catalogue
23 stars 5 forks source link

FC: Collective multiplicities not specified correctly #140

Open DavidGrant-NIWC opened 5 months ago

DavidGrant-NIWC commented 5 months ago

Collective multiplicities are not specified correctly. For instance, FairwaySystem has an AidsToNavigationAssociation which uses a collective multiplicity (there are multiple feature types in the binding) and the lower bound of the collective multiplicity is zero (indicating that there doesn’t have to be an aid associated with the FairwaySystem): image

The DCEG shows this as requiring that one of the possible features must be present (the lower bound of the collective multiplicity is always one, indicating that an aid must be associated with the FairwaySystem): image

This issue seems to be present for every binding which uses a collective multiplicity.

TDYCARHugh commented 5 months ago

I agree but... Some of them had a minimum of 1 before and there was push back (not from me) that during production or editing the collection might be empty at times (during creation or swapping objects in the collection). The discussion, as I understood it, was that we should implement the collective minimums through validation checks. I think the issue might have been more focused on whether an equipment object must have a structure or not and then it was carried through to collections as well.

Either way is ok but I usually prefer to make a tighter model instead of adding more validation checks.

DavidGrant-NIWC commented 5 months ago

Some of them had a minimum of 1 before and there was push back (not from me) that during production or editing the collection might be empty at times (during creation or swapping objects in the collection).

I get that you aren't advocating for this, but for those that are, note that the collection is only expected to satisfy these constraints after editing / production is completed. To include these items in a collection and then set their lower multiplicity to zero doesn't make much sense; why are they in a collection at all?

TDYCARHugh commented 5 months ago

I think the collections should have a min of 1 member. I was paraphrasing the arguments for why they got changed to 0, which did not come from me. I don't remember if it was another Github issue or an email thread where that discussion happened.

kusala9 commented 5 months ago

I'm probably the source of that :-) I noticed the vast preponderance of mandatory associations and asked for them to be taken out so they match what is required from the DCEG. My understanding, having spoken to Jeff at TSM (and before) was that the conditional multiplicity wasn't mandatory and the only mandatory associations were things like IslandGroup and TextPlacement where they make no sense to exist on their own. However, I agree that where an association is mandated by S-58 and we can replicate it in the S-101 FC we should probably try and do so. But, this could create a large overhead for producers if the associations either don't exist or aren't convertible in the source S-57 (but I would have thought they should be?). There's a riusk though that we introduce 100s of validation errors in test data.

I was at the IC-ENC conference with Jeff this week and he showed me the revisions to the associations he's done for the DCEG and I'm not sure the mandatory/optional question is answered yet, but I think we're starting from the point where most are still optional. Maybe we should go through those mandated from S-58 and try to come up with a definitive list. Now the naming convention is agreed upon it would be good to do this before June PT???

kusala9 commented 5 months ago

previous issue on multiplicities is here:

https://github.com/iho-ohi/S-101-Documentation-and-FC/issues/114

gorogara commented 2 months ago

@DavidGrant-NIWC @kusala9 @TDYCARHugh @JeffWootton

Currently, all Collective Multiplicity have been removed from the DCEG. Would it be correct to maintain the value at 0? image

DavidGrant-NIWC commented 2 months ago

The problem originates in the DCEG, you would need @JeffWootton to update the tables.

See https://iho.int/en/s-100wg6-2022 paper and presentation 4.26

image image

When a binding has multiple targets, the multiplicity applies to the collection. So, the following binding indicates that a BridgeAggregation is described by the relationship between a Bridge and the targets: SpanFixed, SpanOpening, Pontoon, and/or PylonBridgeSupport. The FC indicates that zero or more of the targets must be present.

image

The FC is accurately modeling the information from the DCEG (which is wrong): image

The proper way to describe this relationship is via separate bindings:

Feature Association Multiplicity Target(s)
Bridge BridgeAggregation 1:∞ SpanFixed, SpanOpening
Bridge BridgeAggregation 0:∞ Pontoon, PylonBridgeSupport
or alternatively: Feature Association Multiplicity Target(s)
Bridge BridgeAggregation 1:∞ SpanFixed, SpanOpening
Bridge BridgeAggregation 0:∞ Pontoon
Bridge BridgeAggregation 0:∞ PylonBridgeSupport