fix issue #310 by flothesof · Pull Request #313 · ToFuProject/tofu

flothesof · 2019-12-03T15:59:31Z

Proposed changes instead of thos from #311.
This fixes the matplotlib plot error.
Also, I'd like to add a test that triggers the debug plot, but I'm unsure where to put it.
I've just browsed the test files in tofu/tests/tests01_geom but they look really messy... (sorry)
Where should I put my test for this?

codecov-io · 2019-12-03T16:08:58Z

Codecov Report

Merging #313 into devel will increase coverage by 0.32%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##            devel     #313      +/-   ##
==========================================
+ Coverage   41.02%   41.35%   +0.32%     
==========================================
  Files          79       79              
  Lines       23478    23480       +2     
==========================================
+ Hits         9633     9711      +78     
+ Misses      13845    13769      -76

Impacted Files	Coverage Δ
tofu/geom/_def.py	`73.91% <ø> (+7.42%)`	⬆️
tofu/tests/tests00_root/tests03_plot.py	`70.73% <100%> (+1.11%)`	⬆️
tofu/geom/_core.py	`63.6% <75%> (+0.4%)`	⬆️
tofu/geom/_plot.py	`70.44% <0%> (+5.35%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d9b183...51e2e4d. Read the comment docs.

Didou09

Good for me

flothesof · 2019-12-03T16:43:26Z

What would you recommend for tests to make sure this doesn't break again? Which test file should it go in?

Didou09 · 2019-12-03T16:43:40Z

For the test, it should be in tofu/tests/tests01_geom/tests03_core.py
In the test class Test03_Rays(), you can add a method called

def testXX_shortdescription(self):
    pass

Didou09

See comment below

Didou09 · 2019-12-03T16:49:04Z

    ax.set_xlabel(r"X (m)")
    ax.set_ylabel(r"Y (m)")
    ax.set_zlabel(r"Z (m)")
-    ax.set_aspect(aspect="equal", adjustable='datalim')


Good idea,

However, I would recommend keeping the try / except, because 3d plotting in matplotlib is not very stable,
This plot, for instance, used to work fine until a matploltib update.
Given that 3d plotting is not very stable, I would recommend both solution:

Try to make it work with your fix

Keep the try / except as back-up safety in case it stops working for any reason at some point

Can you update your PR by adding the try / except with warning as in PR #311 ?

I understand that aspect="equal" causes the error, but does the second parameter should also be removed?

Good point, I'm going to try.

No, it doesn't work.

ax.set_aspect(adjustable='datalim') TypeError: set_aspect() missing 1 required positional argument: 'aspect'

Ok, it bothers me though to just take out this line. @Didou09 : i'm under the impression that it worked in some cases, no? I have already used this feature of tofu to debug a camera. We could try to replace it with a hack like: https://stackoverflow.com/questions/13685386/matplotlib-equal-unit-length-with-equal-aspect-ratio-z-axis-is-not-equal-to

It may have been a backwards incompatible change in matplotlib no?

flothesof · 2019-12-03T16:50:48Z

Sure, will do this as a next step! Le mar. 3 déc. 2019 à 17:49, Didier <notifications@github.com> a écrit :

…

***@***.**** requested changes on this pull request. See comment below ------------------------------ In tofu/geom/_def.py <#313 (comment)>: > @@ -164,7 +164,6 @@ def Plot_3D_plt_Tor_DefAxes(fs=None, wintit='tofu'): ax.set_xlabel(r"X (m)") ax.set_ylabel(r"Y (m)") ax.set_zlabel(r"Z (m)") - ax.set_aspect(aspect="equal", adjustable='datalim') Good idea, However, I would recommend keeping the try / except, because 3d plotting in matplotlib is not very stable, This plot, for instance, used to work fine until a matploltib update. Given that 3d plotting is not very stable, I would recommend both solution: - Try to make it work with your fix - Keep the try / except as back-up safety in case it stops working for any reason at some point Can you update your PR by adding the try / except with warning as in PR #311 <#311> ? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#313?email_source=notifications&email_token=AASAKXJRZYR22WQCNXQ6F6LQW2EYXA5CNFSM4JU2MRD2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCNZNCRI#pullrequestreview-326291781>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASAKXP72QKG6TL4HCIHOC3QW2EYXANCNFSM4JU2MRDQ> .

pep8speaks · 2019-12-04T09:08:13Z

Hello @flothesof! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file tofu/geom/_core.py:

Line 4435:80: E501 line too long (113 > 79 characters)
Line 4437:80: E501 line too long (84 > 79 characters)
Line 4438:80: E501 line too long (88 > 79 characters)

In the file tofu/tests/tests00_root/tests03_plot.py:

Line 162:80: E501 line too long (82 > 79 characters)
Line 163:80: E501 line too long (84 > 79 characters)

Comment last updated at 2019-12-04 09:27:11 UTC

lasofivec · 2019-12-04T09:15:39Z

-    def compute_dgeom(self, extra=True, plotdebug=True):
+    def compute_dgeom(self, extra=True, show_debug_plot=True):
+        """
+        Computes the dgeom attribute (what does dgeom mean? I'm guessing it stands for shortened dict_geometry?).


I would say so too. I would let @Didou09 confirm :)
It looks like Florian wrote in the doc some questions, would you mind checking them out (also for next parameter)

Do you have any idea what sort of information dgeom contains?

lasofivec · 2019-12-04T09:20:51Z

    ax.set_xlabel(r"X (m)")
    ax.set_ylabel(r"Y (m)")
    ax.set_zlabel(r"Z (m)")
-    ax.set_aspect(aspect="equal", adjustable='datalim')


I understand that aspect="equal" causes the error, but does the second parameter should also be removed?

flothesof · 2019-12-04T09:21:26Z

After further thinking about the issue I've arrived at the conclusion that it's better to not add the try except block.

Supporting a debug plot is a feature that is very useful. It should always work from now on.

The two proposed ways to act are:

change 1: add a try/catch feature that prints an error message if something goes wrong with the debug plot
change 2: add a test to the unit tests that makes sure the debug plot works

The worst-case scenario is that the debug plot breaks somehow in the future.
If this happens:

change 1: a silent warning will be printed to the user who sees this, who might not report it as a bug.
change 2: unit tests will break, prompting the maintainers to fix the bug.

Implementing change 1 and 2 is not useful because change 2 alone will make sure the debug plot works.

Hence my conclusion: change 2 is the way to go.

Didou09 · 2019-12-04T09:49:40Z

Agreed, but:

the bug may very well be matplotlib-version specific
It will work for some users, and not for others, and we cant' force users to upgrade to a specific matplotlib version (too demanding, may break their environment)

=> if we go without the try / except, it means we may have to write tests on the current version of matplotlib to make sure the plot works fine for all versions (i.e.: extra workload).
We can go this way, but I want to be sure everyone is aware of this before we decide

flothesof · 2019-12-04T10:25:26Z

the bug may very well be matplotlib-version specific

As far as I can tell, it is.

It will work for some users, and not for others, and we cant' force users to upgrade to a specific matplotlib version (too demanding, may break their environment)

I don't think it will work for some/not for others. What I am saying is: this is a matplotlib issue, for which we should only write "correct matplotlib code" and not care about the rest. If matplotlib works as expected this will work for everyone.

Didou09 · 2019-12-04T10:34:20Z

I don't think it will work for some/not for others. What I am saying is: this is a matplotlib issue, for which we should only write "correct matplotlib code" and not care about the rest. If matplotlib works as expected this will work for everyone.

Matplotlib will work as expected, except the API may change from one version to another. It is not necessarily a matplotlib bug, more a change of API.
So it will work for those with the good version / API, not for the others, since everyone does not have the same version.
From my point of view, the question is more "If it does not work because of matploltib what do we do?":

Implement a try / except that will make sure it never blocks the computation whatever the API
Implement version / API-specific tests ?

flothesof · 2019-12-04T20:27:52Z

I would say that if we notice a problem, which I think is not super likely, we adjust the required version of matplotlib in the setup.
This allows us to have control over what works while not adding extra complexity.

Didou09 · 2020-02-25T07:25:08Z

This issue was finally adressed in a separate PR #343 .
After some updates on our system here at IRFM, it appears that 3D plottin is definitely not stable / clean with matplotlib and seems to crash in several versions.
Assuming the 3d debug plot will work is too strong an hypothesis.
I decided to remove this 3D plot until either matplotlib gets more stable 3D plot or until we include support for PyQtGraph.
In the meantime, I included a more informative error message.

Update _def.py

9158997

Didou09 self-assigned this Dec 3, 2019

Didou09 self-requested a review December 3, 2019 16:40

Didou09 assigned flothesof and unassigned Didou09 Dec 3, 2019

Didou09 added the geom - bug label Dec 3, 2019

Didou09 approved these changes Dec 3, 2019

View reviewed changes

Didou09 requested a review from lasofivec December 3, 2019 16:42

Didou09 self-requested a review December 3, 2019 16:46

Didou09 requested changes Dec 3, 2019

View reviewed changes

add test and try to make better docstrings

dfab135

Didou09 mentioned this pull request Dec 4, 2019

Issue310 more robust warning for out of vessel los #311

Closed

lasofivec reviewed Dec 4, 2019

View reviewed changes

Update tests03_plot.py

51e2e4d

Didou09 mentioned this pull request Feb 6, 2020

Issue341 indchauto #343

Merged

Didou09 closed this Feb 25, 2020

Conversation

flothesof commented Dec 3, 2019

Uh oh!

codecov-io commented Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Didou09 left a comment

Choose a reason for hiding this comment

Uh oh!

flothesof commented Dec 3, 2019

Uh oh!

Didou09 commented Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Didou09 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flothesof commented Dec 3, 2019 via email

Uh oh!

pep8speaks commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2019-12-04 09:27:11 UTC

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flothesof commented Dec 4, 2019

Uh oh!

Didou09 commented Dec 4, 2019

Uh oh!

flothesof commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Didou09 commented Dec 4, 2019

Uh oh!

flothesof commented Dec 4, 2019

Uh oh!

Didou09 commented Feb 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Dec 3, 2019 •

edited

Loading

Didou09 commented Dec 3, 2019 •

edited

Loading

pep8speaks commented Dec 4, 2019 •

edited

Loading

flothesof commented Dec 4, 2019 •

edited

Loading