theochem / procrustes

Python library for finding the optimal transformation(s) that makes two matrices as close as possible to each other.
https://procrustes.qcdevs.org/
GNU General Public License v3.0
109 stars 20 forks source link

Refactor jupyter notebook for ranking example #90

Closed FanwangM closed 3 years ago

FanwangM commented 3 years ago

This PR refactors ranking with reordering example with procrustes.permutation_2sided Main changes are

PaulWAyers commented 3 years ago

I think we need to be consistent. For Jupyter notebooks I'd tend to favor showing equations throughout, since doing "literate programming" is the whole purpose of Jupyter I think. It is conceivable that some (many?) of our users will interact with the code mainly through Jupyter.

But to the extent that the documentation is merely a recapitulation of the code, it should either agree with the code or link with the code.

PaulWAyers commented 3 years ago

I like this.

We should make the other notebooks like this one, though. The purpose of the notebooks is to make it easier for people to use the code, not to be pedagogical (which is in the paper). I would go back through the other notebooks and make them work the same way: provide a markdown block that explains what happens; put a code block (exactly the same as the paper; I don't think we need extra comments beyond that); add additional markdown explanation at the end; add additional visualization blocks at the end (as needed). This makes it easy for a user to:

Some of the other notebooks do not do this, and I don't think the "fragmented" code is very useful to people who want to use the codeblocks from our notebooks in their own work.

FanwangM commented 3 years ago

I like this.

We should make the other notebooks like this one, though. The purpose of the notebooks is to make it easier for people to use the code, not to be pedagogical (which is in the paper). I would go back through the other notebooks and make them work the same way: provide a markdown block that explains what happens; put a code block (exactly the same as the paper; I don't think we need extra comments beyond that); add additional markdown explanation at the end; add additional visualization blocks at the end (as needed). This makes it easy for a user to:

  • understand what is happening (first markdown block)
  • copy and paste the code block into their own notebook.
  • copy and paste the visualization block into their own notebook.

Some of the other notebooks do not do this, and I don't think the "fragmented" code is very useful to people who want to use the codeblocks from our notebooks in their own work.

The issue is fixed in d7f98fe. I wrapped the code blocks for ranking to be a simple function (code block 2) that could be useful for other people at some point. But this also means that we need to update the corresponding code block in the manuscript. Is it too long for that if we just throw it in the manuscript including the documentation? If not, I will update the mauscript soon.

@PaulWAyers @FarnazH

FanwangM commented 3 years ago

I think this is closest to be a final version. It is great that the exact code snippet from the paper shows up in the notebook in one code block (easy to compare & copy).

@fwmeng88 please:

  1. Remove the image's title and, if needed, reduce its size.
  2. Remove the code block below:
# only run this cell if you need to install the dependices
!pip install git+https://github.com/theochem/procrustes.git@master

Thanks for the summary. The problem has been addressed in 9f09831 and d7f98fe.

FarnazH commented 3 years ago

@fwmeng88 we don't need to have a function for ranking (in the notebook or paper). The code block is simple and easy to adjust. In my opinion, the function and docstrings make it look more complicated, so let's make it match the paper.

Also, the equation below doesn't show up properly (at least when I look at the notebook on GitHub): $$ \label{rank_differential} \mathbf{R}_{n \times n} = \begin{bmatrix} 0 & 1 & 2 & \cdots & n-1 \\ & 0 & 1 & \cdots & n-2 \\ & &\ddots &\ddots & \vdots \\ & & & \ddots & 1 \\ & & & & 0 \end{bmatrix} $$

PaulWAyers commented 3 years ago

Github sometimes doesn't render equations well in notebooks. (A lesson learned the hard way.) Adding blank lines before/after the equation sometimes (but doesn't always) help @fwmeng88 . But if it renders OK in the browser for Jupyter than it is adequate.

FanwangM commented 3 years ago

The short code block is converted back in 2cd2dd5.

For the equation rendering, removing the equation label fixed the problem, bf0a1f1. Please lt me know if you think we need more changes in this PR. @FarnazH @PaulWAyers