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

Correct bound check for axis aligned bounding box and use same formatting for printing cuda or cpu tensor #6444

Merged
merged 4 commits into from
Oct 24, 2023

Conversation

saurabheights
Copy link
Contributor

@saurabheights saurabheights commented Oct 22, 2023

Type

  • Bug fix (non-breaking change which fixes an issue): Fixes a new issue related to printing cuda tensors.
  • New feature (non-breaking change which adds functionality). Resolves Point cloud crop using bounding box #6436
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) Resolves #

Motivation and Context

A. Cuda tensor do not use same formatting as tensor in cpu.
B. Axis aligned Bounding Box can be constructed with invalid bounds and causes aberrant behaviour.

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test
      results
      (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

Description

Printing cuda tensor with same formatting variables with_suffix and indent as passed in calling method is self-explanatory.

For #6436, In case min bound is larger than amx bound in at least one axis, the min bound and max bound values are swapped in those axis/axes with a warning, reporting user of the change made.


This change is Reviewable

@update-docs
Copy link

update-docs bot commented Oct 22, 2023

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@saurabheights
Copy link
Contributor Author

saurabheights commented Oct 22, 2023

image

EDIT - Reworded the message from dimension to axes.

@saurabheights saurabheights force-pushed the khanduja/add_aabb_bounds_check branch from 9381e53 to bfa6f46 Compare October 22, 2023 21:08
@saurabheights saurabheights force-pushed the khanduja/add_aabb_bounds_check branch from bfa6f46 to 0afb67e Compare October 22, 2023 21:10
@ssheorey ssheorey requested a review from benjaminum October 24, 2023 16:13
Copy link
Contributor

@benjaminum benjaminum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 0 of 4 files reviewed, all discussions resolved

@ssheorey ssheorey merged commit 6bbf0ff into isl-org:master Oct 24, 2023
28 of 30 checks passed
saurabheights added a commit to saurabheights/Open3D that referenced this pull request Nov 13, 2023
ssheorey pushed a commit that referenced this pull request Nov 14, 2023
* Change invalid values of min_bound and max_bound as done in legacy, related pr #6444
* Correct points length check when creating AABB from points.
* Print min and max bound of AABB when printing AABB
* Add brief docs for methods and variables, fix warning messages.
* Fix AABB ToString() test
---------
Co-authored-by: Sameer Sheorey <41028320+ssheorey@users.noreply.github.com>
@@ -750,7 +750,7 @@ std::string Tensor::ToString(bool with_suffix,
std::ostringstream rc;
if (IsCUDA() || !IsContiguous()) {
Tensor host_contiguous_tensor = Contiguous().To(Device("CPU:0"));
rc << host_contiguous_tensor.ToString(false, "");
rc << host_contiguous_tensor.ToString(with_suffix, indent);
Copy link
Contributor Author

@saurabheights saurabheights Dec 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I added a regression here. with_suffix should be set to false, else it will print suffix twice. once in inner call where tensor is moved to CPU and once in outer call.

Example -

o3d.core.Tensor.eye(n=4, dtype=o3d.core.Dtype.Float64, device=o3d.core.Device('cuda:0'))
[[1 0 0 0],
 [0 1 0 0],
 [0 0 1 0],
 [0 0 0 1]]
Tensor[shape={4, 4}, stride={4, 1}, Float64, CPU:0, 0x561c38e195d0]
Tensor[shape={4, 4}, stride={4, 1}, Float64, CUDA:0, 0x302000000]

I will address this in my next PR.

saurabheights added a commit to saurabheights/Open3D that referenced this pull request Jan 11, 2024
ssheorey pushed a commit that referenced this pull request Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Point cloud crop using bounding box
3 participants