Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make CoordinateType an "alias" to CoordFloat #110

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nyurik
Copy link
Member

@nyurik nyurik commented Feb 22, 2022

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 georust/geo#746 to simplify such cases

This PR+Release is required for georust/geo#751

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.
@rmanoka
Copy link

rmanoka commented Feb 23, 2022

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
Copy link
Member Author

nyurik commented Feb 23, 2022

@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 georust/geo#742
See also georust/geo#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 added a commit to nyurik/proj that referenced this pull request Feb 23, 2022
This is an alternative, simpler take on georust#110

Adding default constraint allows georust/geo#751
@nyurik
Copy link
Member Author

nyurik commented Feb 23, 2022

I created an alternative PR to this problem -- #111

@rmanoka
Copy link

rmanoka commented Feb 23, 2022

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
Copy link
Member Author

nyurik commented Feb 23, 2022

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
Copy link
Member

michaelkirk commented Mar 21, 2022

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

@nyurik
Copy link
Member Author

nyurik commented Mar 21, 2022

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

@nyurik nyurik marked this pull request as draft March 21, 2022 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants