more extensive readme

[nschloe]

In my experience, it improves the onboarding of your software package if the landing page (usually the github readme) contains all the some information to get started, in particular:

• Has an install section, short and crispy, e.g.,

  On Linux, install the dependencies with

  sudo apt install libcgal-dev

  and then simply install SeismicMesh from pypi,

  pip install SeismicMesh

  An upload to pypi is also strongly advised, this way the user doesn't have to check out the development repo.

• Has a small example that can be copy-pasted.

PS: I was assigned as reviewer of your JOSS paper, hence the flood of suggestions. :)

[krober10nd]

hah :] I figured something was going on here...You're a very good candidate to evaluate this.

I agree with your suggestion regarding pypi. In the past, installation problems have been from users with a weak background in version control and/or python. Pypi would greatly simplify this.

[krober10nd]

I took a stab at this on #20 as well...https://pypi.org/project/SeismicMesh/

[krober10nd]

Looks like the whl is working but when building from a sdist, it can't find CMakeLists.txt when pip calls setuptools.

[nschloe]

The pypi version and the one on github are now different (1.0.0 vs 1.2.something), and also the contents are different. This is not good. I would suggest to use the same README on both platforms, namely via

long_description = file: README.md
long_description_content_type = text/markdown

in setup.py.

Perhaps the following trick is also useful for you: If I decide to make a new release of a package, I bump the version in setup.py/setup.cfg, then I create a "release" on GitHub, then I upload it to PyPi. Because I forget the commands all the time, I put them in a Makefile and just run make publish. See, for example, meshio.

[krober10nd]

Thanks for that. Yes, because my changes are in the open PR that I plan to merge into the main branch.

[nschloe]

Ah okay. Well, it's best practice to make a change in a PR, then merge, then tag and upload (in that order). This is to avoid inconsistencies like right now. (If the PR isn't even merged, the tests might not even pass.)

Also, don't be afraid of tagging/uploading something "wrong". Your unit tests should give you confidence that it's alright, and if you still find an error after releasing (happens to me once in a while, too), you can simply tag/upload a new version.

[krober10nd]

That certainly makes a lot of sense. That's how I'll do it from now on.

[nschloe]

I'd also recommend adding pypi badges,

[![PyPI pyversions](https://img.shields.io/pypi/pyversions/SeismicMesh.svg?style=flat-square)](htt  ps://pypi.org/pypi/SeismicMesh/)
  [![PyPi Version](https://img.shields.io/pypi/v/SeismicMesh.svg?style=flat-square)](https://pypi.or  g/project/SeismicMesh)

and optionally a download count

  [![PyPi downloads](https://img.shields.io/pypi/dm/SeismicMesh.svg?style=flat-square)](https://pypi  stats.org/packages/SeismicMesh)

[nschloe]

The README looks a lot better now. I'll perhaps add a PR to shorten things a bit. It'd be especially useful if the example code was in one block -- users usually want to copy-and-paste one big thing, run it on their computers, done, not having to copy-and-paste 5 different code blocks. If you want comments, I think code comments are fine. Also: syntax-highlighting is a big win!

[krober10nd]

I agree the pypi badges are useful and they were included in #23, which is now merged.

In regard to the README.rst, I made a single block of code with syntax highlighting for Python (looks way better now imo).

[nschloe]

• The downloads badge link is still wrong.

• When copy-pasting the code example, I get

  FileNotFoundError: [Errno 2] No such file or directory: 'velocity_models/Salt_Model_3D/3- D_Salt_Model/VEL_GRIDS/SALTF.ZIP'

  because I haven't downloaded the file. Best specify before the code block that the user needs to do that. It'd be best of nothing had to be unzipped etc. The easier, the better.

• An image of of what the example code produces would be very helpful. From ParaView, you can create screenshots with the mesh only, without the control panel around it. You can also make the background transparent when "screenshotting" from ParaView.

• In the installation section, make it one apt_get command. Also, make the links on text, no need to write it all out.

[krober10nd]

Ok,

1. The download badge is fixed (nb: the package name is all lower case for pypistats).

2. I put in a warning about downloading the velocity model before running the code snippet. I acknowledge your point about easier being better here but I unfortunately cannot ship that data around because of its size.

3. I updated the image of the output there with a much nicer one following your suggestions with ParaView and also connecting more towards the application domain.

4. Now there's only one apt_get command for installation section in the README

5. I made all links hyper references throughout README.rst.

[nschloe]

I put in a warning about downloading the velocity model before running the code snippet.

That's fine. Perhaps, considering the size of the file, you should include "500 MB" in the warning. Even better if the first example could be used with a small file.

Anyway, very nice. Feel free to close this issue if you're happy with the readme; I for one think it looks much better than before.

[krober10nd]

Thanks for the feedback on it; I agree it improved quite a bit! I'll put in a warning there for the download size.

[nschloe]

The path in the readme code example is still wrong.

FileNotFoundError: [Errno 2] No such file or directory: 'velocity_models/Salt_Model_3D/3-D_Salt_Model/VEL_GRIDS/SALTF.ZIP'

[nschloe]

I also get an OOM error

[1]    484887 killed     python3.8 n.py

It seems that the example is not suitable. Reopen please.

[krober10nd]

Ok

1. Just curious, what path did you have to use?

2. Where/when did this OOM occur?

[krober10nd]

Perhaps better to swap it out with a small 2D example there. Smaller download and no chance of this OOM

The EAGE benchmark is a nice example for people who are familiar with this application domain but unfortunately it isn't small.

[krober10nd]

Nevermind. I will fix this shortly.

[krober10nd]

1. Added in a lighter 2D example that runs in either serial or parallel.
2. Fixed the path for the 3D example. I removed velocity_models from the path.
3. I put in a warning about memory usage with the 3D example.

[nschloe]

When following the instructions, I get a rotated output:

Also, I don't see a cell data array that produces the coloring as in the plot. Am I doing something wrong?

As a recommendation -- not a must -- I'd say that you test the code snippets in the readme. Otherwise you'll update the software at some point, forgetting the example in the readme. Then first-time users will hit a wall and never touch seismicmesh again. If the first user experience isn't great, it will be the last user experience. There's exdown for extracting code from markdown files, I use it in quadpy for example. Perhaps there's something similar for RST, or you want to reformat your readme to markdown. Anyway, like I said, this is just a suggestion.

[krober10nd]

I did test the code snippets in the README and I state below the figures:

"Above shows the mesh from running the code below. Note, the seismic velocity data has been interpolated onto the vertices of the mesh."

This orientation is by design and is detailed here on the website: https://seismicmesh.readthedocs.io/en/par3d/tutorial.html#file-i-o-and-visualization-of-meshes

[nschloe]

I did test the code snippets in the README

Of course you tested it once, I'm talking about integration testing. Perhaps that's what you mean, too. (Not sure now.)

Note, the seismic velocity data has been interpolated onto the vertices of the mesh

The data is not in the VTK file that is written out by the program, right?

This orientation is by design

Then the images in the readme should show this orientation, too.

[krober10nd]

Yes, I tested it once, but, no, I don't test the README.rst in the CI, I'm not sure actually how that could be done.

For our numerical models that use the code, they expect the data in this format (z,x,y) which is why I did it. However, for visualization, it's customary to see it in this (x,y,z) plane.

So what I could do is update the example in the README.rst so the vtk is rotated into x,y,z to match the screenshot images there and put a comment

[nschloe]

Yes, I tested it once, but, no, I don't test the README.rst in the CI, I'm not sure actually how that could be done.

The way I described above. I promise you that if you don't test the snippets, they will stop working after the next PR. :smile:

For our numerical models that use the code, they expect the data in this format (z,x,y) which is why I did it.

You mean some downstream code, right? This shouldn't be SeismicMesh's concern then. I just want to make sure that if a first-time copy-and-pastes the code from the readme, she gets exactly the results that you're showing. Important here is: How to get the coloring? The data is in included in the output VTK.

[krober10nd]

The way I described above. I promise you that if you don't test the snippets, they will stop working after the next PR. 😄

Ah I see. 👍 Something to learn then.

You mean some downstream code, right? This shouldn't be SeismicMesh's concern then. I just want to make sure that if a first-time copy-and-pastes the code from the readme, she gets exactly the results that you're showing.

Yes, the orientation is for some downstream codes and is a convenience feature. I'll switch the orientation in the examples and README.rst so they appear (x,y,z) and put on notes.

Important here is: How to get the coloring? The data is in included in the output VTK.

So the interpolation of the velocity onto the mesh (what you see on the images) is what's done in a downstream code that uses the mesh (and some HDF5 file it outputs). This interpolation routine is not a part of SeismicMesh because we're using a solver that the user can select arbitrary spatial orders (Firedrake codes). We thus do the interpolation onto cells/DoFs of varying spatial order at execution time, not at mesh generation time.

The idea behind showing the colors is it accentuates the point that we're meshing seismic velocity models and that's why I put the note that I interpolated the velocity onto the vertices.

[nschloe]

So the interpolation of the velocity onto the mesh

Ah okay, I see; you're using some other software to add this data then. I'd suggest to remove those images then and only shot what SeismicMesh can do.

[krober10nd]

Ok sure thing. I also switched to a markdown format so I can do the unit tests with exdown. Do you think I should then remove the 3D example from the README there since it won't be able to run on the unit tests because of its size (or is there some way to ignore that code when testing)?

[nschloe]

Good question. You can prepend the code block with <!--exdown-skip--> (see the readme), so that's probably a good idea. It won't get tested then, of course, but one test is better than none.

[krober10nd]

Sweet thanks.

[krober10nd]

Okay. I switched out the images for colorless ones to reflect what you see is what you get.

I switched to markdown (md) for the README and used your exdown package to run the 2D example Python code on there when doing CI (https://github.com/krober10nd/SeismicMesh/blob/revisions2/tests/test_README.py)

I put in notes when writing the mesh to disk about the orientation (and switched all examples to (x,y,z) standard format including on the README.md).

[nschloe]

The mesh is still rotated, but that seems to hard to fix now. Let's close this.

[krober10nd]

Ah whoops. It was just a simple change I forgot.