Skip to content

fix issue #310 #313

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.

Sign up for GitHub

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

Closed
flothesof wants to merge 3 commits into from
Closed

fix issue #310 #313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9158997
Update _def.py
flothesof Dec 3, 2019
dfab135
add test and try to make better docstrings
flothesof Dec 4, 2019
51e2e4d
Update tests03_plot.py
flothesof Dec 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions tofu/geom/_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4430,10 +4430,16 @@ def _compute_kInOut(self, largs=None, dkwd=None, indStruct=None):
indout[0, :] = indStruct[indout[0, :]]
return kIn, kOut, vperp, indout, indStruct

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?).
Copy link
Copy Markdown
Collaborator

@lasofivec lasofivec Dec 4, 2019

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor Author

@flothesof flothesof Dec 4, 2019

Choose a reason for hiding this comment

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

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


:param extra: bool, whether to perform some extra computations (which ones?)
:param show_debug_plot: bool, whether to show a debugging plot for invalid LOSes
"""
# Can only be computed if config if provided
if self._dconfig["Config"] is None:
msg = "Attribute dgeom cannot be computed without a config !"
msg = "Attribute dgeom cannot be computed without a config!"
warnings.warn(msg)
return

Expand All @@ -4444,13 +4450,13 @@ def compute_dgeom(self, extra=True, plotdebug=True):
# Perform computation of kIn and kOut
kIn, kOut, vperp, indout, indStruct = self._compute_kInOut()

# Clean up (in case of nans)
# Check for LOS that have no visibility inside the plasma domain
ind = np.isnan(kIn)
kIn[ind] = 0.0
ind = np.isnan(kOut) | np.isinf(kOut)
if np.any(ind):
kOut[ind] = np.nan
msg = "Some LOS have no visibility inside the plasma domain !\n"
msg = "Some LOS have no visibility inside the plasma domain!\n"
msg += "Nb. of LOS concerned: %s out of %s\n" % (
str(ind.sum()),
str(kOut.size),
Expand All @@ -4460,10 +4466,9 @@ def compute_dgeom(self, extra=True, plotdebug=True):
msg += "\nIndices of LOS with no visibility:\n"
msg += repr(ind.nonzero()[0])
warnings.warn(msg)
if plotdebug:
if show_debug_plot:
PIn = self.D[:, ind] + kIn[None, ind] * self.u[:, ind]
POut = self.D[:, ind] + kOut[None, ind] * self.u[:, ind]
# To be updated
_plot._LOS_calc_InOutPolProj_Debug(
self.config,
self.D[:, ind],
Expand Down
1 change: 0 additions & 1 deletion tofu/geom/_def.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Copy link
Copy Markdown
Member

@Didou09 Didou09 Dec 3, 2019

Choose a reason for hiding this comment

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

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 ?

Copy link
Copy Markdown
Collaborator

@lasofivec lasofivec Dec 4, 2019

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@flothesof flothesof Dec 4, 2019

Choose a reason for hiding this comment

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

Good point, I'm going to try.

Copy link
Copy Markdown
Contributor Author

@flothesof flothesof Dec 4, 2019

Choose a reason for hiding this comment

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

No, it doesn't work.

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

Copy link
Copy Markdown
Collaborator

@lasofivec lasofivec Dec 4, 2019

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@flothesof flothesof Dec 4, 2019

Choose a reason for hiding this comment

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

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

return ax


Expand Down
21 changes: 21 additions & 0 deletions tofu/tests/tests00_root/tests03_plot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,3 +156,24 @@ def test01_plot_shotoverview(self):
# All together, with conf
kh = tf._plot.plot_shotoverview(self.dobj, config=self.conf)
plt.close('all')


def test_camera_missing_LOS_debug_plot():
"""This test is meant to test issue #310. It creates a camera with missing LOS
which should trigger a working debug plot to help the user adjust the camera."""

# using a "simple" configuration (not many internal structures)
# so that rendering of the debug plot is fast
config = tf.geom.utils.create_config('NSTX')

cam2d = tf.geom.utils.create_CamLOS2D(
config=config,
P=[3.4, 0, 0],
N12=10,
F=0.1,
D12=0.1,
angs=[np.pi, np.pi / 6, 0],
Name="Dummy",
Exp="Dummy",
Diag="None",
)