CRS Refactor (Add Datum, CoordinateOperation, CoordinateSystem classes; update CRS repr) #263
Conversation
If you are talking about inheritance, I had the same thought about maybe doing a mixin base class for both CRS and Datum. I hesitated because it might be complicated with cython, but probably a worthwhile venture to attempt to reduce some duplication. |
|
If you weren't referring to inheritance, there are cases where the datum is None (see the tests). |
|
Yes, I wasn't really referring to inheritance (although that would be a good idea for some common functionality), but to the actual properties.
That's indeed a problem for my suggestion. So I think this is the case for BoundCRS or CompoundCRS. The general question here is also to what extent we want to give access to specific properties of those (like getting access to the underlying CRSs) |
snowman2
changed the title
snowman2
changed the title
|
I updated it again for your review - will probably add more later to address other issues. |
snowman2
force-pushed
the
datum_class
branch
3 times, most recently
from
4801852
to
7f9800c
Compare
snowman2
force-pushed
the
datum_class
branch
2 times, most recently
from
1669c0e
to
c8fe36d
Compare
|
Another thing to note is that I use horizontal_datum as backup for datum which is useful for the compound CRS. |
datum/ellipsoid/prime_meridian
|
@jorisvandenbossche, after going through all of this, I am so glad you brought this up. This is much better than it used to be and has a much better design. |
snowman2
changed the title
snowman2
force-pushed
the
datum_class
branch
3 times, most recently
from
4ff0fc5
to
0fb4cff
Compare
…; added coordinate system to CRS repr
There was a problem hiding this comment.
Small comment (didn't yet go through again in more detail)
For me, the main API question that I am wondering is the CRS class. Is a single class enough, or do we want more classes? I think at least customizing the repr depending on which CRS type it is, would be useful (will comment about that in the repr issue). But of course, that can also be done without having CRS subclasses.
But, regarding getting this merged: how do we go about that? Because it is getting a big PR, making it difficult to do other PRs touching the CRS at the moment.
So it might be good to merge this relatively fast (after proper review of course). But meaning that not all details need to be perfect. Eg we can further tweak the repr in a separate PR afterwards, ..
| "CoordinateOperation", | ||
| "Datum", | ||
| "Ellipsoid", | ||
| "PrimeMeridian", |
There was a problem hiding this comment.
Is there a reason that CoordinateSystem is not in this list?
(because there is no way to create it from the class itself?)
There was a problem hiding this comment.
But I see below that you are including them in the sphinx docs (in the _crs namespace), so maybe we could add it to this namespace nevertheless? (so the namespace in sphinx is the same as for the other objects)
There was a problem hiding this comment.
because there is no way to create it from the class itself?
Yes, that is correct. No user facing way to create it. So, only access is through the CRS class. I prefer not to include it for clarity of usage.
I see below that you are including them in the sphinx docs
I am including it so the user knows what they are getting from the CRS class. Just a reference for them when looking at the return types from the CRS class.
Great idea for another issue.
Currently just waiting on @jswhit for approval. Once he approves, it should be good to go. |
Also, want to decide on what to do with the towgs84. I am leaning towards removing it. |
It can also certainly be add later, so I would also remove it for now. |
|
Set towgs84 from params instead of the PROJ method. |
There was a problem hiding this comment.
I haven't had time to look at this in detail, but it looks like @jorisvandenbossche has done a thorough review. @snowman2, if you're happy with this now please go ahead and merge.
|
Sounds good. Thanks for the review @jswhit! |
|
Sounds good, thanks @jswhit! |
|
Great, thanks a lot for this work, @snowman2 ! (side remark: given that this are quite some changes, and also additions to the API, should the next version rather be 2.2.0 instead of 2.1.4 ?) |
If we wait to release until after PROJ 6.1.0 (May 1st) and add the option to keep the axis ordering, then bumping to 2.2.0 makes sense to me. |
@jorisvandenbossche
Addresses:
#262
#264
#267
#265
#274