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

Updated mPDF tutorial materials #11

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

benfrandsen
Copy link
Contributor

I have recently updated the mpdf directory within the cmi_scripts directory, and I would like to merge these changes into the master branch. I ended up removing some of the python scripts and replacing them with jupyter notebooks. I haven't made any changes to any of the other directories.

@sbillinge
Copy link

Ben, I took a quick look and here are some lightning comments.

  1. Who is your target audience. It would have to be a fairly advanced pythonic person because there are really no instructions about how to get going other than "start with the tutorial notebook", but how the hell do I do that if I am not a python person? How do I download it? How do I run it? If I know how to clone a git repo and have a conda envo installed with jupyter in it I am ok, but if I am just a non-pythonic person with a windows computer....it won't be clear to me what the next steps are after reading "start with the tutorial notebook"
  2. Likewise with the "explore from there", it would be helpful in my efforts to choose what to do next if there was a small amount of info about what the different examples exemplify, maybe in the readme.md
  3. Along these lines....what link are you going to provide to people? Is it this link?:
    https://github.com/diffpy/cmi_exchange/tree/master/cmi_scripts/mpdf
    In that case it is not a very "friendly" splash page, and I may not even know to scroll down and read the readme. Actually, this is a generic problem with cmi-exchange, not really your problem, but also a contributing reason why noone uses CMI and cmi-exchange. Probably something friendlier would be to have a cmi-exchange.github.io page that we link to that welcomes people and helps them with next steps. @pavoljuhas do you think that this is a good idea? In that case @benfrandsen you could write the lightweight mPDF welcom docs somewhere in that docs tree, pointing back to the jupyter notebook which is the kind of "getting started" docs. Generic info about how to get started using ipython etc. coul dthen be better shared across projects in the exchange.

@dragonyanglong
Copy link

Hi @benfrandsen , sorry for the delay. I will organize my comments and questions when I went through the tutorial, from the view of a new user for mPDF. Should be done before tomorrow.

@pavoljuhas
Copy link
Member

... but how the hell do I do that if I am not a python person? How do I download it? How do I run it?

@sbillinge - the instructions how to run a notebook are in the main README file for the project. They need some update, because the start command is now jupyter notebook rather than ipython notebook. In addition there is no back-link to the main CMI project. I suggest we follow up on that at the ancient open issue #2 - I am going to summarize the necessary changes for the main README there.

@benfrandsen - could you please add a quick installation instructions for diffpy.mpdf and link
to http://www.diffpy.org/products/mPDF.html to its mpdf/README.md file?
Also, please reformat the README text so that its lines wrap at at most 78 characters.

I will check the PR more carefully later, but thus far I found these small points:

use UNIX line ends for text files

I have already fixed that for the data files in your PR. Please apply that fix using:

git pull https://github.com/pavoljuhas/cmi_exchange.git for-ben

use topic-named branches for PR

(relevant for future PRs)

A good practice for starting a new PR is to create a new branch with a descriptive name,
say jupyter-notebooks-for-mpdf-demo, and start it of the latest master branch in the main repo:

git remote add upstream https://github.com/diffpy/cmi_exchange.git
git fetch upstream master
git checkout -b some-descriptive-name upstream/master

This helps to avoid merge conflicts, since you'd be starting your work from the last version, and is also more descriptive once your branch gets merged. (Compare commit message Merge branch 'master' of ... vs. Merge branch 'jupyter-notebooks-for-mpdf-demo' of ....)

@sbillinge
Copy link

Thanks @pavoljuhas
@benfrandsen please could you link to the cmi-exchange readme in your mpdf docs where installation of ipython etc. is discussed
@dragonyanglong please can you pretend you are someone like Liz and see if you (she) can follow the instructions there for installing everything and getting it going? If not, please can you make a branch and a PR with edits to make those instructions at the right level?

Thx.

@dragonyanglong
Copy link

Hi @benfrandsen , congrats on the new baby, Abel!!

I just gathered all my edits, comments, and questions when I went through your tutorial jupyter notebooks as a new user of mPDF. I didn't find an efficient way to make edits on jupyter notebooks, in which you can still git diff to see my edits. So I created a new branch on my fork, and wrote all my edits in Markdown cells below the related part in the same jupyter notebooks. Please see 3cfb3f5 You can search "#long#" one by one throughout these tutorial jupyter notebooks introTutorial.ipynb, example_corefinement.ipynb, and example_refineSpinDir.ipynb , because I use "#long#" as starting for every of my edits for you to easy distinguish my edits in notebook.

For instructions and installing, it was good to me and also looked straightforward to users I think. Just one quick point for you reference, if acting as a totally new python and jupyter notebook user, it might be good to add some words like "Please change jupyter kernel into the environment where user install mPDF package, which is named as diffpy according to your installing instruction", before importing the python package on the top of scripts. Since usually people's default jupyter kernel environment is not the one with mPDF installed. for a totally new user, it might be a little depressive that he is stuck at the error (package not exist) when just run the first command in the tutorial.

Thank you!

@dipankas
Copy link

dipankas commented Dec 4, 2020

Hi
I am using diffpyCMI on virtual box ubuntu on win 10 machine. But if i try to run mpdf tutorials the following error shows up.
Could you please help me in this regard?
Thank you
Dipankar


In [1]: %run example_fromPDFgui.py

ModuleNotFoundError                       Traceback (most recent call last)
~/Desktop/mPDF/example_fromPDFgui.py in
     13 from scipy.optimize import leastsq
     14
---> 15 from diffpy.mpdf import *
     16 from diffpy.Structure import loadStructure
     17

ModuleNotFoundError: No module named 'diffpy.mpdf'

@dragonyanglong
Copy link

Hi @dipankas , mpdf module needs to be installed separately from the diffpy-cmi package.

Did you run conda install diffpy.mpdf -c benfrandsen to install it yet?

@dragonyanglong
Copy link

It seems like this PR is ready to be merged, right? @sbillinge @benfrandsen

@benfrandsen
Copy link
Contributor Author

@dragonyanglong I believe the current version of the tutorial materials are NOT compatible with the python3 version of diffpy, so I should update that before the merge happens. Would it be better just to delete this PR and start a new one altogether?

@sbillinge
Copy link

I need to catch up with our current status of diffpy-cmi releasing. Do we have a P3 release? If we do, is it the main product that we are pointing people to?

@dipankas
Copy link

@dragonyanglong
Thank you. It is not able to install mpdf module since I have python 3.8.

@benfrandsen
Copy link
Contributor Author

@dragonyanglong
Thank you. It is not able to install mpdf module since I have python 3.8.

The version on my personal github page at https://github.com/benfrandsen/diffpy.magpdf is compatible with python 3. You can download and install from source inside your existing diffpy environment.

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.

5 participants