Skip to content
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

Remove Pyplot and refactor plotting to object-oriented plotting #187

Merged
merged 73 commits into from
Mar 13, 2019

Conversation

antmoev
Copy link
Contributor

@antmoev antmoev commented Mar 12, 2019

Will resolve #167. (link to old PR: #177 closed because of erroneous branch name)

This issue remove matplotlib.pyplot from the source code and make plotting object-oriented (OO).
Now drawing, displaying, or plotting functions return a matplotlib.figure.Figure and that's up to the caller to decide whether to save or display the figure.
In addition, the main three notebooks: notebooks/guide_dataset_building.ipynb, notebooks/training_guideline.ipynb, notebooks/performance_metrics.ipynb were updated to improve OO functionalities.

Files that were updated:
AxonDeepSeg/morphometrics/compute_morphometrics.py : remove pyplot and updtate to OO
AxonDeepSeg/morphometrics/launch_morphometrics_computation.py: remove unused matplotlib imports
AxonDeepSeg/testing/segmentation_scoring.py: remove pyplot and update to OO
AxonDeepSeg/visualization/visualize.py: remove pyplot and update to OO
test/morphometrics/test_compute_morphometrics.py: update to handle new functions signature
notebooks/guide_dataset_building.ipynb: update to OO
notebooks/training_guideline.ipynb: update to OO
notebooks/performance_metrics.ipynb: update to OO

====================================================================

In `compute_morphometrics.py`:
The `display_axon_diameter` function is now renamed to
`draw_axon_diameter` to improve meaning.

Add function `save_map_of_axon_diameters` to save the map to
a certain folder.
Then modified all necessary files to be compatible with the
updates.
@coveralls
Copy link

coveralls commented Mar 12, 2019

Coverage Status

Coverage increased (+0.3%) to 81.846% when pulling 99c9274 on maf88/167-move-to-OO-plotting into 9810954 on master.

def test_score_analysis_runs_successfully_with_visualization_on(self):

tmp_folder = "/tmp"
Copy link
Member

@mathieuboudreau mathieuboudreau Mar 13, 2019

Choose a reason for hiding this comment

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

I think this folder might not work in Windows. Better off creating a temp directory relative to your current directory (e.g. "__tmp_segmentation_scoring__/" for it to be specific to this function), then deleting that entire directory at the end. At least then, if it doesn't get cleaned up, we'll know about it (instead of being dumped in a folder that's not version controlled). That's how I've been managing it anyways (I've had an issue where Windows would not let me delete files that I had explicitly created as "temp" files, using the tempfile module. If you feel strongly about using the tmp/ folder, I could re-install a Windows virtual machine on my laptop and test it on there.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. We should definitely use the tempfile module

Copy link
Member

Choose a reason for hiding this comment

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

Sure, we can give it a go again, I'll just have to re-explore some workarounds for Windows, because some of its functionality was making tests failing on that OS back when I was first testing it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's do something simple because I'm going to refactor score_analysis so the metrics evaluation is separated from the visualization.

Copy link
Member

Choose a reason for hiding this comment

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

How about not deleting temp file if permissions don’t allow? They will eventually be deleted by the OS at some point

@mathieuboudreau mathieuboudreau self-requested a review March 13, 2019 00:42
Copy link
Member

@mathieuboudreau mathieuboudreau left a comment

Choose a reason for hiding this comment

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

Great, thanks @maf88 ! Only thing left to do is add a small description of these changes are added to the changelog for the upcoming version (2.2) both here and here.

@antmoev
Copy link
Contributor Author

antmoev commented Mar 13, 2019

Now I need to update the wiki to clarify how we should implement plotting functionalities.

…axondeepseg into maf88/167-move-to-OO-plotting
@antmoev antmoev changed the title Maf88/167 move to oo plotting Remove Pyplot and refactor plotting to object-oriented plotting Mar 13, 2019
@mathieuboudreau mathieuboudreau merged commit 05cb434 into master Mar 13, 2019
@mathieuboudreau mathieuboudreau deleted the maf88/167-move-to-OO-plotting branch March 13, 2019 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace pyplot with OO matplotlib API
4 participants