Skip to content

Issue310 more robust warning for out of vessel los#311

Closed
Didou09 wants to merge 1 commit intodevelfrom
Issue310_MoreRobustWarningForOutOfVesselLOS
Closed

Issue310 more robust warning for out of vessel los#311
Didou09 wants to merge 1 commit intodevelfrom
Issue310_MoreRobustWarningForOutOfVesselLOS

Conversation

@Didou09
Copy link
Copy Markdown
Member

@Didou09 Didou09 commented Dec 3, 2019

Main changes:

  • Try / except introduced in tf.geom.Rays._compute_dgeom() in case where some LOS have no visibility inside vessel to make sure an error during the plotting process does not stop computation
  • Exception is still caught and displayed as warning

Issues:

@Didou09 Didou09 self-assigned this Dec 3, 2019
@Didou09 Didou09 changed the base branch from master to devel December 3, 2019 13:24
@Didou09 Didou09 mentioned this pull request Dec 3, 2019
Copy link
Copy Markdown
Contributor

@flothesof flothesof left a comment

Choose a reason for hiding this comment

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

I've just looked at your commit.
It looks alright to me.
However, encapsulating with a try except something that should work and actually help the user debug his camera is of great value.
It turns out that the 3D axis does not support the axis = equal option (see matplotlib/matplotlib#15382).
So fixing this instead of putting it into a try/except block would be a better way to address this, don't you think?
I'd advocate for fixing this at the core instead of your proposal.

@flothesof
Copy link
Copy Markdown
Contributor

flothesof commented Dec 3, 2019

I've just found out where the problem is. Taking out line 167 in tofu/geom/_def.py solves the issue.

ax.set_aspect(aspect="equal", adjustable='datalim')

I'm also thinking it would be great to add a unit test with a camera that has missing LOS so that our tests checks that the debug plot works.

@flothesof flothesof mentioned this pull request Dec 3, 2019
@lasofivec
Copy link
Copy Markdown
Collaborator

Should we close this PR then ?

@Didou09
Copy link
Copy Markdown
Member Author

Didou09 commented Dec 4, 2019

Yes, you're right, PR #313 will do, this one is no use anymore

@Didou09 Didou09 closed this Dec 4, 2019
@Didou09 Didou09 deleted the Issue310_MoreRobustWarningForOutOfVesselLOS branch December 2, 2020 14:00
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