Update documentation and warnings before 0.1 release
#502
Conversation
There was a problem hiding this comment.
Wow, such an improvement to the documentation!! Super great job @rhugonnet!
I have some questions, generally pertaining to the non-documentation part of the PR (perhaps in the future changes like this could be a separate PR? :) )
xdem/coreg/base.py
Outdated
| @@ -2627,7 +2701,7 @@ def copy(self: CoregType) -> CoregType: | |||
| """Return an identical copy of the class.""" | |||
| new_coreg = self.__new__(type(self)) | |||
|
|
|||
| new_coreg.__dict__ = {key: copy.deepcopy(value) for key, value in self.__dict__.items() if key != "pipeline"} | |||
| new_coreg.__dict__ = {key: copy.copy(value) for key, value in self.__dict__.items() if key != "pipeline"} | |||
There was a problem hiding this comment.
What does this change do?
| @@ -848,6 +788,34 @@ def to_matrix(self) -> NDArrayf: | |||
| """Convert the transform to a 4x4 transformation matrix.""" | |||
| return self._to_matrix_func() | |||
|
|
|||
| def to_translations(self) -> tuple[float, float, float]: | |||
| """ | |||
| Convert the affine transformation matrix to only its X/Y/Z translations. | |||
There was a problem hiding this comment.
I think something funky will happen here if scaling is involved (I don't remember which order the components are applied in). I feel like a warning should be added to this and the to_rotations() saying something like "Warning: Information may be lost".
And the conversion is less of a conversion and more of an extraction I would say! Since there's no validation that scaling won't mess with the result.
There was a problem hiding this comment.
I agree that it's not a conversion, the idea was just to have an extraction! We could change this in the description?
Leaving this conversation open for @adehecq to comment.
There was a problem hiding this comment.
And I'll look into it for when scaling is involved, great point 😉
Ongoing
TO-DO:
BlockwiseCoregto coregistration page,.metakeys for users (and make the attribute public in a separate PR),.info()method forCoregPipeline,BlockwiseCoreg,0.1,Resolves #477 (last step)
Resolves #505
Resolves #464
Resolves #434
Resolves #285
Resolves #275
Resolves #431
Resolves #532
Resolves #528
Resolves #562
Resolves #583