Skip to content

Issue202 spectro x2 d crystal#320

Merged
lasofivec merged 29 commits intodevelfrom
Issue202_SpectroX2DCrystal
Dec 18, 2019
Merged

Issue202 spectro x2 d crystal#320
lasofivec merged 29 commits intodevelfrom
Issue202_SpectroX2DCrystal

Conversation

@Didou09
Copy link
Copy Markdown
Member

@Didou09 Didou09 commented Dec 18, 2019

Intermediate PR to update devel with new features regarding 2D X-Ray spectrometer.

These new features are in tofu/geom/_core_optics.py, _comp_optics.py and _plot_optics.py.

They were added as extra files because they include a lot of new things (routines, classes) and because this way it does not affect the basic classes.
They are not unit-tested yet.

This is an update just to make available some functionalities and avoid a single huge PR at the end. But there is no user (except me) for these at this point, so it won't affect anyone.

VEZINET Didier and others added 24 commits December 2, 2019 17:29
…me() and get_local_noute1e2() and fallback to self._DEFLAMB in self._checkformat_bragglamb() if nothing provided
…cing ddist, dtheta, dpsi to optimize position
…ble nphifit and nlambfit, started adding magaxis phi computation => notes
…discretize circle and select in interval of interest
…etector not tangential to Rowland circle ! needs to be fixed in _get_detect_approx()
…a, with source, consider lamb-dependent tab? saved in cryst
…lotting routines and get 2d data for Quartz crystal
… a single lambda, and computation from plasma points started with calc_psidthetaphi_from_pts_lamb()
… ids wall, and fully recursive to_dict for .mat saving
@Didou09 Didou09 requested a review from lasofivec December 18, 2019 11:10
@Didou09 Didou09 self-assigned this Dec 18, 2019
@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Dec 18, 2019

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-12-18 11:39:41 UTC

Copy link
Copy Markdown
Member Author

@Didou09 Didou09 left a comment

Choose a reason for hiding this comment

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

In addition to my comments:

  • Several routines are unfinished but have no impact on unit tests and other classes
  • Several routines are already quite useful (for me as a user)
  • a npz file has been added, but it is temporary (useful for developping and debugging), but it will be removed before the release and merge to master

Comment thread tofu/geom/_core.py
pinhole = dgeom['D'][:, 0] + k[0]*u[:, 0]
dgeom['pinhole'] = pinhole

if np.any(np.isnan(dgeom['D'])):
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This change to _core.py is just adding a safeguard (making it more robust with respect to ill-defined input from users)

Comment thread tofu/imas2tofu/_core.py
user=user, tokamak=tokamak, version=version,
ref=ref, isget=isget, get=get)

def add_ids_for_synthdiag(self, ids=None, occ=None, idd=None,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Minor changes also to make it more robust

Comment thread tofu/utils.py

def load_from_imas(shot=None, run=None, user=None, tokamak=None, version=None,
ids=None, Name=None, out=None, tlim=None, config=None,
ids=None, Name=None, returnas=None, tlim=None, config=None,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

out keyword replaced by returnas for clarity (will do it gradually everytime I see an opportunity)

Comment thread tofu/utils.py
# -------------------
# Prepare ids
assert ids is None or type(ids) in [list,str]
if type(ids) not in [list, str]:
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Minor change for robustness

Comment thread tofu/utils.py
dout = dout[shot[0]]['Data'][0]
elif nshot == 1 and nPla == 1:
dout = dout[shot[0]]['Plasma2D'][0]
return out
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Was not returning the good variable...

Comment thread tofu/utils.py
deep = 'dict' if deep else 'ref'
if sep is None:
sep = _SEP
if mode == 'mat':
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If a .mat file is being saved, the separator must be '_', otherwise a '.' would corrupt the mat file (minor change for robustness)

@codecov-io
Copy link
Copy Markdown

codecov-io commented Dec 18, 2019

Codecov Report

Merging #320 into devel will decrease coverage by 0.67%.
The diff coverage is 6.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##            devel     #320      +/-   ##
==========================================
- Coverage   40.89%   40.22%   -0.68%     
==========================================
  Files          79       79              
  Lines       23553    23998     +445     
==========================================
+ Hits         9633     9653      +20     
- Misses      13920    14345     +425
Impacted Files Coverage Δ
tofu/imas2tofu/_core.py 0.69% <0%> (ø) ⬆️
tofu/version.py 100% <100%> (ø) ⬆️
tofu/utils.py 47.93% <18.6%> (-0.07%) ⬇️
tofu/geom/_plot_optics.py 5.85% <3.38%> (-1.11%) ⬇️
tofu/geom/_core.py 63.17% <33.33%> (-0.03%) ⬇️
tofu/geom/_comp_optics.py 8.46% <4.44%> (-4.36%) ⬇️
tofu/geom/_core_optics.py 14.69% <6.27%> (-3.83%) ⬇️

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 ad51d41...c31f86b. Read the comment docs.

@lasofivec lasofivec merged commit d698343 into devel Dec 18, 2019
@lasofivec
Copy link
Copy Markdown
Collaborator

@Didou09 I'll let you see if you want to keep the branch or not. As I understood, the changes are not over

@lasofivec
Copy link
Copy Markdown
Collaborator

I just realized this PR introduced a *.npz file in the root directory. I will erase it in a new PR...

@Didou09 Didou09 mentioned this pull request Jan 30, 2020
@Didou09 Didou09 mentioned this pull request Mar 10, 2020
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.

4 participants