Update view_corr_mat in visualisation commands and Update the introduction page in docs #108

wingedRuslan · 2019-04-10T08:05:37Z

I'm ready to merge

What's the context for this pull request?
Solves issue update view_corr_mat in visualisation commands #83 and issue More explicit linking to autodoc content within documentation #106
What's new?

The function view_corr_mat in visualisation_commands is redesigned so that one can pass a correlation matrix (pandas dataframe) and plot that, or a filename instead and plot the matrix saved in the filename. In the function body I check the type of the argument.
I did not want to add more arguments(corr_mat, output, corr_mat_file=None) to the function, because in this case if a user wants to use a filename, a function call would be view_corr_mat(None, output-file, filename).
Passing None as an argument could be unclear and intimidating for non-coders. Besides, calling the function would be different for 2 cases:
view_corr_mat(corr_mat-object, output-file) - to plot correlation matrix (pandas DataFrame)
view_corr_mat(output-file, filename) - to plot correlation matrix (saved in file)

But if you believe that including 2 arguments is better, please let me know, and I will make appropriate changes :)

Getting started section is modified and updated in the introduction page.

highlighted that we have automatically generated docstring documentation
added links to the submodules page ,the index, the module index pages.
readers are informed that they can use the search bar to look up functions

What should a reviewer feedback on?
The decision regarding the view_corr_mat function
Is the updated introduction page clear for users?
Does anything need to be updated after merge?
The project's website -> introduction page

…DataFrame object and PathToFile

wingedRuslan · 2019-04-10T13:49:30Z

I have built the sphinx documentation locally before submitting the pull request. And it shows the docs as expected.

KirstieJane

Fab! Thanks @wingedRuslan! I'll ping @Islast and see if she wants to weigh in, and if you're up for adding a test for the corr_mat file that would be pretty fab...but otherwise it all looks good to me 🚀

KirstieJane · 2019-04-15T19:17:30Z

scona/scripts/visualisation_commands.py

+
+    Parameters
+    ----------
+    corr_mat : :class:`pandas.DataFrame` or :class:`str`


This logic makes sense to me! Thanks for clearly explaining your thought process in the PR 😸

I also agree with your reasoning here, fewer arguments is great, and your changes to the docstrings are very clear.

KirstieJane · 2019-04-15T19:18:51Z

scona/scripts/visualisation_commands.py


    # If cost is given then roughly threshold at that cost.
    # NOTE - this is not actually the EXACT network that you're analysing
    # because it doesn't include the minimum spanning tree. But it will give
    # you a good sense of the network structure.
    # #GoodEnough ;)

+    if isinstance(corr_mat, str):
+        M = np.loadtxt(corr_mat)                  # Read in the data


Feels like we should add a test for this, what do you think @wingedRuslan?

If that feels a bit too much then we can just open an issue noting that we should check the dimensions of the file when we load it in. Up to you and @Islast ✨

I think we can leave np.loadtxt to do the heavy lifting on properly importing data. Or did you mean it's worth checking the file isn't too large before trying to import it? That's something I've never really considered

Sorry folks! Massively unclear on my part.

The test that @wingedRuslan has added further down to check that the data is square is what I was thinking of, not anything about reading in the data properly 😬

docs/source/introduction.rst

Islast

Nice work!:hibiscus::tada: I'm very pleased with the changes to the docs, I think you've done a really good job of translating my slightly vague ideas about making it easier to navigate for a newcomer into reality.

The changes to the code are also really good. I agree wholeheartedly with the changes you've made, but I have a couple of additional requests

I would like to support numpy arrays as well as pandas dataframes as input to this function.
This is one I don't feel strongly about and am happy for you to go with your gut, but I have made a comment further down about how I usually to do type-checking, just in case you aren't familiar with raising errors in python. It's a useful tool to have.

I really appreciate the work you've put into this @wingedRuslan

scona/scripts/visualisation_commands.py

Islast · 2019-04-16T06:27:17Z

scona/scripts/visualisation_commands.py

+    elif isinstance(corr_mat, pd.DataFrame):
+        M = corr_mat.to_numpy()                   # Convert the DataFrame to a NumPy array
+    else:
+        print("Please provide correlation matrix as pandas.DataFrame object or as a path to the file containing the matrix")


I like type checking a whole lot. My go to for handling these cases is raising a type error. This is nice because printed output can often get lost. In this case raising an exception would look like:

else: raise TypeError("corr_mat argument must be a pandas.DataFrame object or as a path to the file containing the matrix")

My reasoning for doing things this way is that if this command is run with a whole load of other code, it's easy for a printed message to get lost. Raising an error message aborts the execution of code (unless there is an exception for it specified) and the printed output will end with

File: "somefile", line somenumber, TypeError corr_mat argument must be a pandas.DataFrame object or as a path to the file containing the matrix

which gives the user a lot of useful information to help debug

If you come across a case when you want to convey some information to the user about how the code is being executed, but you don't want to abort the code, raising a warning is another good trick.

You've got a point! 👍
I will adjust the type checking by raising a type error.

Islast · 2019-04-16T06:29:35Z

scona/scripts/visualisation_commands.py

+
+    Parameters
+    ----------
+    corr_mat : :class:`pandas.DataFrame` or :class:`str`


I also agree with your reasoning here, fewer arguments is great, and your changes to the docstrings are very clear.

docs/source/introduction.rst

Islast · 2019-04-16T06:49:04Z

scona/scripts/visualisation_commands.py


    # If cost is given then roughly threshold at that cost.
    # NOTE - this is not actually the EXACT network that you're analysing
    # because it doesn't include the minimum spanning tree. But it will give
    # you a good sense of the network structure.
    # #GoodEnough ;)

+    if isinstance(corr_mat, str):
+        M = np.loadtxt(corr_mat)                  # Read in the data


I think we can leave np.loadtxt to do the heavy lifting on properly importing data. Or did you mean it's worth checking the file isn't too large before trying to import it? That's something I've never really considered

…ing a TypeError if unsupported type provided

KirstieJane

Fab! Thanks @wingedRuslan!

Last question - do we check anywhere that all the values are floats? I feel like I can’t see that but I’m also doing this on my phone and might just have missed it. The biggest error is going to be if people read in a file that has column and row labels (in my best guess opinion) so a check for that would be great...

KirstieJane · 2019-04-17T07:53:39Z

scona/scripts/visualisation_commands.py

+    elif isinstance(corr_mat, np.ndarray):
+        M = corr_mat                              # support numpy array as input to the function
+    else:
+        raise TypeError("corr_mat argument must be a 1)pandas.DataFrame object or 2) numpy.array or 3)a path to the file containing the matrix")


Tiny typo - can you add a space after 1) and 3)?

Oh - and I just thought, maybe re-order the text or the order of the arguments so that 1, 2 and 3 correspond to the order of the if statements above?

sure, I will re-order the text in TypeError message and fix typos in the next commit

KirstieJane · 2019-04-17T07:54:06Z

scona/scripts/visualisation_commands.py

+    else:
+        raise TypeError("corr_mat argument must be a 1)pandas.DataFrame object or 2) numpy.array or 3)a path to the file containing the matrix")
+
+    if M.shape[0] != M.shape[1]:


This is what I was thinking of! Great stuff!!

KirstieJane · 2019-04-17T07:55:31Z

scona/scripts/visualisation_commands.py


    # If cost is given then roughly threshold at that cost.
    # NOTE - this is not actually the EXACT network that you're analysing
    # because it doesn't include the minimum spanning tree. But it will give
    # you a good sense of the network structure.
    # #GoodEnough ;)

+    if isinstance(corr_mat, str):
+        M = np.loadtxt(corr_mat)                  # Read in the data


Sorry folks! Massively unclear on my part.

The test that @wingedRuslan has added further down to check that the data is square is what I was thinking of, not anything about reading in the data properly 😬

Islast

Everything looks great, merge when ready

wingedRuslan added 5 commits April 2, 2019 23:12

Try to link to autodoc content in the introduction.rst

0cb64ab

Add documentation to the introduction page

86ff8d6

Add documentation to the introduction page, ready for PR

1500c32

Merge branch 'master' of https://github.com/wingedRuslan/scona

5412842

Update view_corr_mat in visualisation commands to accept corr_mat as …

9d083dc

…DataFrame object and PathToFile

KirstieJane reviewed Apr 15, 2019

View reviewed changes

Islast requested changes Apr 16, 2019

View reviewed changes

wingedRuslan added 2 commits April 16, 2019 16:16

Add support of numpy.array as an input for view_corr_mat and add rais…

d93b9fe

…ing a TypeError if unsupported type provided

Check that corr_mat is n x n matrix, otherwise raise Error

d9ed917

KirstieJane reviewed Apr 17, 2019

View reviewed changes

Explain corr_mat argument to view_corr_mat() more precisely

f9fe78c

wingedRuslan requested a review from Islast April 18, 2019 08:40

Islast approved these changes Apr 18, 2019

View reviewed changes

wingedRuslan merged commit 8dfb892 into WhitakerLab:master Apr 18, 2019

Islast mentioned this pull request May 29, 2019

update view_corr_mat in visualisation commands #83

Closed

wingedRuslan mentioned this pull request May 31, 2019

Review of functions in make_figures.py #113

Open

wingedRuslan mentioned this pull request Aug 20, 2019

View_corr_mat() - make new || leave old #149

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update view_corr_mat in visualisation commands and Update the introduction page in docs #108

Update view_corr_mat in visualisation commands and Update the introduction page in docs #108

wingedRuslan commented Apr 10, 2019 •

edited

Loading

wingedRuslan commented Apr 10, 2019 •

edited

Loading

KirstieJane left a comment

KirstieJane Apr 15, 2019

Islast Apr 16, 2019

KirstieJane Apr 15, 2019

Islast Apr 16, 2019

KirstieJane Apr 17, 2019

Islast left a comment

Islast Apr 16, 2019

wingedRuslan Apr 16, 2019

Islast Apr 16, 2019

Islast Apr 16, 2019

KirstieJane left a comment

KirstieJane Apr 17, 2019

KirstieJane Apr 17, 2019

wingedRuslan Apr 17, 2019

KirstieJane Apr 17, 2019

KirstieJane Apr 17, 2019

Islast left a comment

Update view_corr_mat in visualisation commands and Update the introduction page in docs #108

Update view_corr_mat in visualisation commands and Update the introduction page in docs #108

Conversation

wingedRuslan commented Apr 10, 2019 • edited Loading

wingedRuslan commented Apr 10, 2019 • edited Loading

KirstieJane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Islast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KirstieJane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Islast left a comment

Choose a reason for hiding this comment

wingedRuslan commented Apr 10, 2019 •

edited

Loading

wingedRuslan commented Apr 10, 2019 •

edited

Loading