Make CoordinateType an "alias" to CoordFloat

[nyurik]

Migrate CoordinateType: Float + Copy + PartialOrd + Debug to CoordinateType: CoordFloat.

TBD: Does it make sense to get rid of CoordinateType outright, and replace it with CoordFloat?

This simplifies usage of proj in other geo libs by making trait requirements consistent. The old requirement of Float + Copy + PartialOrd + Debug is now expanded to Float + Num + Copy + NumCast + PartialOrd + Debug, plus it fully moves the management of this type upstream to geo-types - so once Default is added to geo-types, it becomes transparently added to proj as well.

See also https://github.com/georust/geo/discussions/746 to simplify such cases

This PR+Release is required for https://github.com/georust/geo/pull/751

[rmanoka]

Feature based type definition doesn't seem good. It could lead to confusion if the feature is enabled by some dependencies and not explicitly by the user.

Is there a particular functional advantage to this?

[nyurik]

@rmanoka initially i was not even aware that the proj lib could be built without using geo-types. I was trying to introduce Default as a constraint into geo-types (see this pr), and that didn't work because geo/geo uses proj which uses geo-types, and since proj does not directly use CoordFloat, it doesn't compile. Thus, I tried to make proj use centrally defined CoordFloat that would have simplified everything... But alas, CI informed me that proj can be built without geo-types, so I introduced cfg-based alternative -- far from ideal, but it is really hard to have a system where sometimes you use common types, and sometimes you do not.

The main goal is to implement https://github.com/georust/geo/pull/742 See also https://github.com/georust/geo/discussions/746 about overall project structure to make it a bit easier to work with

P.S. Technically all this could work even without relying on the centrally defined types, but that would require constantly maintaining CoordFloat definition in two places (like it was done before). Not ideal, but doable.

[nyurik]

I created an alternative PR to this problem -- https://github.com/georust/proj/pull/111

[rmanoka]

By feature-based trait defn., I mean that the definition of the trait is changing depending on the features that are enabled. Since cargo accumulates all the features enabled across dependencies, the geo-types feature might be enabled on proj even though the user did not explicitly enable it in the top-level Cargo.toml. We need to ensure that this will not be cause any suprises (eg. a compilation error because some type that would have been T: CoordinateType is no longer implementing that trait). For this PR, I don't see an obvious problem, but it takes more time to reason that there is no issue at all.

Note that both this, and the similar changes in geo repo are technically breaking changes. We should document these in the change-log. Typically, breaking changes that are somewhat of a cosmetic nature (as opposed to adding support for a concrete use-case) are more subjective to review as we'll need to reason about what this would for a possible future use-case. For instance, is it reasonable to imagine a CoordNum that doesn't have a Default. Do other packages (such as lin-alg. crates, etc.) that have a "scalar" trait include Default?

Turning this around, why is it important to add Default to our scalar types? Note, that I'm not discouraging this PR, but just trying to reason its importance.

[nyurik]

By feature-based trait defn., I mean that the definition of the trait is changing depending on the features that are enabled. Since cargo accumulates all the features enabled across dependencies, the geo-types feature might be enabled on proj even though the user did not explicitly enable it in the top-level Cargo.toml. We need to ensure that this will not be cause any suprises (eg. a compilation error because some type that would have been T: CoordinateType is no longer implementing that trait). For this PR, I don't see an obvious problem, but it takes more time to reason that there is no issue at all.

Thanks for the explanation, I wasn't aware of aggregate nature of the features.

Note that both this, and the similar changes in geo repo are technically breaking changes. We should document these in the change-log. Typically, breaking changes that are somewhat of a cosmetic nature (as opposed to adding support for a concrete use-case) are more subjective to review as we'll need to reason about what this would for a possible future use-case. For instance, is it reasonable to imagine a CoordNum that doesn't have a Default. Do other packages (such as lin-alg. crates, etc.) that have a "scalar" trait include Default?

I suspect you meant some other lib, not lin_alg :). Looking at a big linear alg matrix multiplication code, I see they only require absolute minimum, individually tailored to each feature (i.e. creating matrix has different T constraint from multiplying a matrix). I think this is an interesting approach, but not sure if it will work with geo due to a larger number of moving parts (separate crates, multilevel data, etc).

Turning this around, why is it important to add Default to our scalar types? Note, that I'm not discouraging this PR, but just trying to reason its importance.

Always a good point, and it did make me rethink it. Defaults were discussed and implemented a while back, but they were done as an impl on Coordinate<T>, not the T itself. It makes creating such coords easier in some scenarios, without using T::zero() (which is another trait available on T). My main goal is to make it possible to experiment with the new data type architecture, so perhaps the introduction of Default could be even postponed. My current struggle is to work around const generic limitation with fixed array sizes and their serialization. Maybe it could be done differently. Regardless, I do want to thank you for reviewing!

[michaelkirk]

@nyurik - can you summarize what you'd like to happen with this PR? At this point is it redundant with edit: #11? #111

[nyurik]

Converting to draft until it is certain that this change is needed. See also #111 P.S. Yes, it is mostly redundant to #111 - an neither approaches are certain to be needed just yet, so tabling it for now