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

FIX: Viz test correction #1374

Merged
merged 18 commits into from Nov 9, 2017

Conversation

Projects
None yet
5 participants
@skoudoro
Member

skoudoro commented Nov 2, 2017

The goal of this PR is to update some tests for VTK 7.1.x. This PR:

  • Resolve tests recording on windows (Permission Error)
  • Update disk_slider_2d recorded_events
  • Replacing vtkVolumeTextureMapper2D by vtkSmartVolumeMapper

it should resolve #1282.

Can you have a look and test it @ranveeraggarwal @MarcCote @dmreagan?

Jon Haitz Legarreta Gorroño and others added some commits Jul 30, 2017

DOC: Fix typos and formatting in .rst files and Python examples.
Fix typos and formatting in .rst files and Python examples'
documentations:
- Fix typos in files.
- Replace tabs for two white spaces.
- Fix errors in citation and DIPY internal cross-refs.
- Add missing cross-refs (e.g. Aganj MRM 2010).
- Add links where necessary (ISBI HARDI Contest 2013 and FreeSurfer).
- Fix LaTeX math formatting errors.
- Make the example/DIPY code object markdown consistent (inverted commas).
- Capitalize the acronym long names' first/corresponding letters (e.g.
  Constrained Spherical Deconvolution).
- Capitalize acronyms (e.g. FA).
- Make the writing of terms consistent in terms of uppercase/lowercase
  (e.g. b0 vs. B0; cospus callosum vs. Corpus Callosum, etc.).
- Use the [NameYear] convention for citations.
- Place all citations under a 'References' section, and use the subsection
  markdown for the section.
- Use inverted commas consistently to reference code objects.
- Make the b-value units consistent across files (s/mm^2).
- Create and include missing figures for 'reconst_fwdti.py'.
- Write DIPY instead of any of its variants in the documentation.
- Link the first appearance of dipy in every file (use dipy_).
DOC: Format README.rst file code blocks.
Use the reST syntax to highlight code blocks in README.rst when describing
the DIPY installation commands.
improve open / close management of "recorded_events.log" file. This a…
…void "Permission Error: [WinError 32]" on windows

@skoudoro skoudoro referenced this pull request Nov 2, 2017

Closed

Tests fail on viz module #1282

@skoudoro

This comment has been minimized.

Member

skoudoro commented Nov 2, 2017

Because of this line, I get this error on python 2.7 :

SyntaxError: cannot delete variable 'recorder' referenced in nested scope

This syntax is authorized in python 3.6. I do not see how I can manage that. Indeed, the recorder files need to be closed before the end of this with InTemporaryDirectory() directive. Any idea for this python problem @arokem @Garyfallidis @MarcCote @ranveeraggarwal ?

@ranveeraggarwal

This comment has been minimized.

Member

ranveeraggarwal commented Nov 3, 2017

recorder is being referenced in the function _stop_recording_and_close. If you delete it outside its scope, it'll cause an error if the function is called after deletion.
I did a little research on this topic, and it seems that Python 2.7.x is safer when it comes to this. Python 3.2+ tells you to take care of it yourself.
Solution: we'll need to change the way this is done.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Nov 3, 2017

While at it, can you describe what you've recorded for test_ui_disk_slider. You can put it in a comment like I did for UI ListBox (see https://github.com/nipy/dipy/pull/1355/files#diff-cbb804b6eece2deed9da8e763c2a7e76R359).

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Nov 3, 2017

While testing your patch, I realized that using protocol=-1 in pickle.save (in test_ui.py) causes some backward compatibility issues. Protocol 4 is not available in Python 2.7. I think we should use protocol=2 instead. Could you make the modification in this PR and re-save your .pkl using protocol 2? Thanks.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Nov 3, 2017

Regarding the del recorder issue. Why do we need the file to be close at that specific point? Is it a Windows issue?

@skoudoro

This comment has been minimized.

Member

skoudoro commented Nov 3, 2017

Because with InTemporaryDirectory(): delete the entire directory tree at the end of this function. Unfortunately, recorder does not close the file before this deletion and it raises the following error:

Permission Error: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\koudoro\\AppData\\Local\\Temp\\tmpm1x9w6uj\\recorded_events.log'

Otherwise, ok for pickle and comment on test @MarcCote

@codecov-io

This comment has been minimized.

codecov-io commented Nov 3, 2017

Codecov Report

Merging #1374 into master will decrease coverage by 0.03%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1374      +/-   ##
==========================================
- Coverage   87.04%   87.01%   -0.04%     
==========================================
  Files         228      228              
  Lines       29077    29086       +9     
  Branches     3129     3131       +2     
==========================================
- Hits        25311    25310       -1     
- Misses       3059     3068       +9     
- Partials      707      708       +1
Impacted Files Coverage Δ
dipy/viz/tests/test_ui.py 83.53% <0%> (ø) ⬆️
dipy/viz/window.py 64.22% <0%> (-0.64%) ⬇️
dipy/viz/tests/test_window.py 93.6% <100%> (+0.05%) ⬆️
dipy/viz/tests/test_fvtk.py 91.46% <100%> (+0.1%) ⬆️
dipy/viz/fvtk.py 61.36% <50%> (-0.29%) ⬇️
dipy/viz/ui.py 89.86% <0%> (-0.48%) ⬇️

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 26413a2...0e80bc3. Read the comment docs.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Nov 3, 2017

LGTM

@@ -514,7 +514,7 @@ def _stop_recording_and_close(obj, evt):
self.iren.Start()
# Deleting this object is the unique way
# to close the file.
del recorder
recorder = None

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Nov 4, 2017

Member

In the _stop_recording_and_close shouldn't we have something like:

if (recorder):
    recorder.stop()

to prevent crash because of nullref?
Or is it that that scenario won't happen?

This comment has been minimized.

@MarcCote

MarcCote Nov 4, 2017

Contributor

From what I understand self.iren.Start() is blocking until the user closes the window at which point the ExitEvents are handled before releasing the block. So, it should happen that recorder is None inside _stop_recording_and_close.

But, yeah, we could add the check to be on the safe side because something VTK still surprises me :P.

This comment has been minimized.

@skoudoro

skoudoro Nov 4, 2017

Member

ahaha :-) , @dmreagan told me to do that and I gave the same answer as @MarcCote.

No problem, I will add this test when I will get access to my computer.

@Garyfallidis Garyfallidis merged commit 991b279 into nipy:master Nov 9, 2017

1 of 3 checks passed

codecov/patch 25% of diff hit (target 87.04%)
Details
codecov/project 87.01% (-0.04%) compared to 26413a2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@skoudoro skoudoro deleted the skoudoro:fix-viz-1282 branch Nov 16, 2017

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment