search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
706 stars 37 forks source link

[REVIEW]: emg3d: A multigrid solver for 3D electromagnetic diffusion. #1463

Closed whedon closed 5 years ago

whedon commented 5 years ago

Submitting author: @prisae (Dieter Werthmüller) Repository: https://github.com/empymod/emg3d Version: v0.7.1 Editor: @jedbrown Reviewer: @akelbert, @emersodb, @lukeolson Archive: 10.5281/zenodo.3339421

Status

status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/d559f2dbd8538007937797122887df0c"><img src="http://joss.theoj.org/papers/d559f2dbd8538007937797122887df0c/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/d559f2dbd8538007937797122887df0c/status.svg)](http://joss.theoj.org/papers/d559f2dbd8538007937797122887df0c)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@akelbert & @emersodb & @lukeolson, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @jedbrown know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @akelbert

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @emersodb

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @lukeolson

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @akelbert, it looks like you're currently assigned as the reviewer for this paper :tada:.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands
whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

jedbrown commented 5 years ago

@akelbert @emersodb @lukeolson :wave: Welcome and thanks for agreeing to review! The comments from @whedon above outline the review process, which takes place in this thread (possibly with issues filed in the emg3d repository). I'll be watching this thread if you have any questions.

prisae commented 5 years ago

Great to see the review started! I repeat a question here that I asked in the original issue (https://github.com/openjournals/joss-reviews/issues/1431#issuecomment-489406698): "I saw one typo and some of the reference-bracketing in the compiled version did not turn out as I intended. Is there a way to edit them now or shall I wait for the reviews first?"

danielskatz commented 5 years ago

You can make changes in the paper.md and paper.bib - after you have done so, enter a new comment here containing @whedon generate pdf to rebuild the pdf

prisae commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

prisae commented 5 years ago

I pushed a revised version. The only thing I did was changing things a little to take into account that citations in JOSS seem to always have brackets (). So I avoided having (()), and tried to make sentences that do not have a (citation) in its flow. Nothing change with regards to the context.

emersodb commented 5 years ago

Not a super important issue, but when doing the pip install, the version number installed was emg3d-0.6.1 rather than 0.5.0 as indicated in the version. Should we be forcing an install of 0.5.0 for the review?

prisae commented 5 years ago

Since I submitted the manuscript I worked on it and released v0.6.0 and v0.6.1. I don't know what is the normal procedure in JOSS, if using the most recent version to final publication date or the version when it was submitted. Asking @jedbrown ?

jedbrown commented 5 years ago

It's typical to review the most recent release or maintenance/stable release. When revisions occur as a result of review (also typical) then the final archived version will be tagged post-review (could be v0.6.2, for example). Assuming you would like v0.6.1 to be reviewed, I'll update that now.

jedbrown commented 5 years ago

@whedon set v0.6.1 as version

whedon commented 5 years ago

OK. v0.6.1 is the version.

emersodb commented 5 years ago

A minor issue that I ran into while working through the 1c_3D_triaxial_SimPEG example is that, as I use pip as my python package manager, pymatsolver does not automatically include Pardiso since MKL is not packaged within pip distributions as it is in Anaconda. So I needed to do a pip install MKL before the example could be run using Pardiso

emersodb commented 5 years ago

In the examples repository, it appears that the folder SEG-EAGE/' is missing from the data/ folder. Thus, when I try to run the 2a_SEG-EAGE_3D-Salt-Model.ipynb notebook, it yields that exception

FileNotFoundError: [Errno 2] No such file or directory: './data/SEG-EAGE/SALTF.ZIP'

prisae commented 5 years ago
prisae commented 5 years ago

You can get the data from the SEG-website or via the direct link. The zip-file is ~1.2 GB big. Unzip the archive, and place the file salt_and_overthrust_models/3-D_Salt_Model/VEL_GRIDS/SALT.ZIP (20.0 MB) into ./data/SEG-EAGE/ (or adjust the path in the following cell).

emersodb commented 5 years ago

Ah, apologies on the data-side. I missed that comment in the notebook!

prisae commented 5 years ago

No problem at all.

I just added the following sentence under requirements in the notebook 1c_3D_triaxial_SimPEG.ipynb:
«Note, in order to use the Pardiso-solver pymatsolver has to be installed via conda, not via pip!»
Thanks for pointing that out!

prisae commented 5 years ago

v0.6.2 is out. All these new versions are changes under the hood to improve CPU and RAM usage, nothing changes for the user-facing functionality. I do not expect further new versions until the JOSS review is finished (if it is not taking too long).

emersodb commented 5 years ago

I may have missed this in the examples or elsewhere in the repository, so please correct me if I'm wrong, but I didn't notice if there were any examples exhibiting optimal scaling for the MG solver. It would be beneficial, I think, to add an example showing linear scaling of the solver, even for just a very simple setup. Otherwise, I think the examples do a good job of verifying the algorithm against existing solvers.

prisae commented 5 years ago

That is a good and timely question @emersodb. Since version 0.6.2 (so two days ago) I had a CPU & RAM section in the docs, and a notebook to estimate memory. Given your question I expanded this section and also added a notebook to estimate runtime.

Figures

runtime

RAM-Usage

emersodb commented 5 years ago

At this time, I believe that my review is complete and would recommend publication of the emg3d submission.

prisae commented 5 years ago

Thanks for your time, input, and feedback @emersodb

prisae commented 5 years ago

:question: We want to cite this article. I assume this review is not done within a week or two, so what is the preferred way of citing a JOSS article under review? Just the usual "submitted to JOSS"?

In the proof, the article has already a DOI. I assume this will remain the same and is not a dummy-DOI for the time being, correct?

Adjusted version from the proof:

Werthmüller et al., (2019). emg3d: A multigrid solver for 3D electromagnetic diffusion. Submitted to the Journal of Open Source Software, 4(37), 1463; https://doi.org/10.21105/joss.01463.

jedbrown commented 5 years ago

That DOI hasn't been created yet, but that is the DOI that will be created upon acceptance. Do you intend to cite this JOSS paper from a new submission or from a camera-ready accepted manuscript?

prisae commented 5 years ago

That is fine, then it is as I understood it. It hasn't been created, but it will be created exactly like this (10.21105/joss.01463) once it is accepted and in its final form.

It is a new submission; it will still take some time until this will be in print or published. I hope/expect that by then this review will be finished and the DOI created.

lukeolson commented 5 years ago

@jedbrown do you still need the review from me? If so, then I can give it a look today.

jedbrown commented 5 years ago

@lukeolson Yes, please.

prisae commented 5 years ago

I thought I'd ask about the status of the review, as over five weeks passed since the review started.

akelbert commented 5 years ago

Yes, I'm very sorry for the delay. I haven't been able to install the software yet because of the summer travel. Also, this is a new process for me and I haven't figured it out yet. I'm on my way to the airport again right now. I'll try to provide something within a week, but if you need to proceed without my review I would also understand.

Anna

Sent from my android device.

-----Original Message----- From: "Dieter Werthmüller" notifications@github.com To: openjournals/joss-reviews joss-reviews@noreply.github.com Cc: Anna Kelbert anya@coas.oregonstate.edu, Mention mention@noreply.github.com Sent: Tue, 25 Jun 2019 5:57 AM Subject: [EXTERNAL] Re: [openjournals/joss-reviews] [REVIEW]: emg3d: A multigrid solver for 3D electromagnetic diffusion. (#1463)

I thought I'd ask about the status of the review, as over five weeks passed since the review started.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/openjournals/joss-reviews/issues/1463#issuecomment-505412348

lukeolson commented 5 years ago

I believe everything is done on my checklist. One suggestion was to add a basic example of usage: https://github.com/empymod/emg3d/issues/14

prisae commented 5 years ago

Thanks @lukeolson for your review!

Re basic example: Is the one basic example that is already in the docs under the Getting started section not enough? I think it is pretty similar to yours: https://emg3d.readthedocs.io/en/stable/usage.html#example

lukeolson commented 5 years ago

Yes, that's probably fine. I agree my small example is nearly the same in the end.

prisae commented 5 years ago

One more question if you don't mind @lukeolson: I saw from your website that you are part of PyAMG, so I assume that you have experience in multigrid methods. Do you have any feedback from the multigrid-viewpoint in particular?

jedbrown commented 5 years ago

@akelbert Is there anything we can do to facilitate your review? Please note that it is fine to review from the perspective of a potential user for joint inversion; other reviewers have covered different perspectives and you need not spend time in the overlap if time is limited. Thanks.

prisae commented 5 years ago

from the perspective of a potential user for joint inversion

There is no (joint) inversion in emg3d, I assume you simply meant "from the perspective of a potential user" or "from the perspective of a potential user for modelling".

jedbrown commented 5 years ago

@prisae Yes, understood that it isn't a feature of emg3d, but a researcher might want to use emg3d in conjunction with other software for that purpose. We can still ask whether it can do what would be needed for such uses without getting in the way of aspects that are out of scope. It isn't so much about the particular case of joint inversion, but that its demands are representative of other research applications that may go beyond the scope of the simulation tool and direct uses planned by authors. Note that such capability is by no means mandatory for publication, but this is a good opportunity for constructive feedback.

prisae commented 5 years ago

:+1: Thanks for the clarification @jedbrown . I wasn't sure if it was a typo or meant exactly like that.

akelbert commented 5 years ago

Basically, just not my environment. As I said, I'm not a Python person! So obvious things aren't obvious for me. In trying to update the packages to

conda install -c prisae emg3d

I completely broke my anaconda so cannot update everything else that I need to install emg3d.  I know this doesn't belong in this review, but what do I do when

akelbert$ conda install -c prisae emg3d Traceback (most recent call last):   File "/Users/akelbert/Developer/local/miniconda3/lib/python3.6/site-packages/conda/exceptions.py", line 1043, in call     return func(*args, **kwargs)   File "/Users/akelbert/Developer/local/miniconda3/lib/python3.6/site-packages/conda/cli/main.py", line 84, in _main     exit_code = do_call(args, p)   File "/Users/akelbert/Developer/local/miniconda3/lib/python3.6/site-packages/conda/cli/conda_argparse.py", line 80, in do_call     module = import_module(relative_mod, name.rsplit('.', 1)[0])   File "/Users/akelbert/Developer/local/miniconda3/lib/python3.6/importlib/init.py", line 126, in import_module     return _bootstrap._gcd_import(name[level:], package, level)   File "", line 978, in _gcd_import   File "", line 961, in _find_and_load   File "", line 950, in _find_and_load_unlocked   File "", line 655, in _load_unlocked   File "", line 678, in exec_module   File "", line 205, in _call_with_frames_removed   ...   File "/Users/akelbert/Developer/local/miniconda3/lib/python3.6/ctypes/init.py", line 366, in getitem     func = self._FuncPtr((name_or_ordinal, self)) AttributeError: dlsym(0x7f8173d645e0, archive_read_open_filename_w): symbol not found

Also, this review environment is new to me and I couldn't find an explanation of how to use it. I don't even spend time on github (use other types of version control). All the above, conflated with an insane amount of travel... bad combination.

If I could easily install emg3d I'd be happy to spend some time running it and providing feedback. Dear authors, I sincerely apologize for making you wait! Unless @jedbrown is able to help me out, you'd be better off proceeding without my input for now.

On 6/30/19 9:12 PM, Jed Brown wrote:

@akelbert https://github.com/akelbert Is there anything we can do to facilitate your review? Please note that it is fine to review from the perspective of a potential user for joint inversion; other reviewers have covered different perspectives and you need not spend time in the overlap if time is limited. Thanks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1463?email_source=notifications&email_token=AB7NQ2NYFH76WNU6H2BUSN3P5FYY3A5CNFSM4HODB6GKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY434AY#issuecomment-507100675, or mute the thread https://github.com/notifications/unsubscribe-auth/AB7NQ2NGBNIEDTCJMMZO6DDP5FYY3ANCNFSM4HODB6GA.

jedbrown commented 5 years ago

Thanks, @akelbert. I usually recommend pip over anaconda because it's so common for conda users to accidentally mix up which Python is used. Can you try python3 -m pip install --user emg3d? This worked well for me. There are tests in the main repository, but the examples might be a better place for you to look at usage. If you have Jupyter (it can also be installed using pip), you can run example notebooks here: https://github.com/empymod/emg3d-examples

These examples depend on the discretize package, which can be installed via python3 -m pip install --user discretize. I cloned the examples repository, opened the minimal working example, and was able to reproduce successfully, for example.

(@prisae I did encounter Javascript Error: IPython is not defined on cells 5 and 8 when using Jupyter Lab; it works fine with Jupyter Notebook. Is there a missing import/magic or does that widget not work with Lab?)

akelbert commented 5 years ago

@jedbrown, thanks for the response! Any ideas?

akelbert$ python3 -m pip install --user emg3d Collecting emg3d   Could not fetch URL https://pypi.python.org/simple/emg3d/: There was a problem confirming the ssl certificate: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:749) - skipping   Could not find a version that satisfies the requirement emg3d (from versions: ) No matching distribution found for emg3d

On 7/2/19 9:57 PM, Jed Brown wrote:

Thanks, @akelbert https://github.com/akelbert. I usually recommend pip over anaconda because it's so common for conda users to accidentally mix up which Python is used. Can you try |python3 -m pip install --user emg3d|? This worked well for me. There are tests in the main repository, but the examples might be a better place for you to look at usage. If you have Jupyter (it can also be installed using pip), you can run example notebooks here: https://github.com/empymod/emg3d-examples

These examples depend on the discretize https://pypi.org/project/discretize/ package, which can be installed via |python3 -m pip install --user discretize|. I cloned the examples repository, opened the minimal working example https://github.com/empymod/emg3d-examples/blob/master/0a_Minimum_working_example.ipynb, and was able to reproduce successfully, for example.

(@prisae https://github.com/prisae I did encounter |Javascript Error: IPython is not defined| on cells 5 and 8 when using Jupyter Lab; it works fine with Jupyter Notebook. Is there a missing import/magic or does that widget not work with Lab?)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1463?email_source=notifications&email_token=AB7NQ2NUUTNKYWM2WLSVF3LP5QPRPA5CNFSM4HODB6GKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZDF3HA#issuecomment-507927964, or mute the thread https://github.com/notifications/unsubscribe-auth/AB7NQ2J5FRQDTYROPD42SS3P5QPRPANCNFSM4HODB6GA.

jedbrown commented 5 years ago

For our reference, can you paste the output from python3 -m pip --version? I think this answer will resolve the cert issue.

akelbert commented 5 years ago

akelbert$ python3 -m pip --version pip 9.0.1 from /Users/akelbert/Developer/local/miniconda3/lib/python3.6/site-packages (python 3.6)

akelbert$ pip install --trusted-host pypi.org --trusted-host files.pythonhosted.org emg3d Collecting emg3d   Could not fetch URL https://pypi.python.org/simple/emg3d/: There was a problem confirming the ssl certificate: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:749) - skipping   Could not find a version that satisfies the requirement emg3d (from versions: ) No matching distribution found for emg3d

akelbert$ pip install --trusted-host pypi.org --trusted-host files.pythonhosted.org pip setuptools Requirement already satisfied: pip in /Users/akelbert/Developer/local/miniconda3/lib/python3.6/site-packages Requirement already satisfied: setuptools in /Users/akelbert/Developer/local/miniconda3/lib/python3.6/site-packages

Somehow I've never had luck with python :(

On 7/2/19 10:14 PM, Jed Brown wrote:

For our reference, can you paste the output from |python3 -m pip --version|? I think this answer https://stackoverflow.com/a/29751768/33208 will resolve the cert issue.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1463?email_source=notifications&email_token=AB7NQ2PN5TT5WUIMYMH5Y4DP5QRSFA5CNFSM4HODB6GKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZDGSII#issuecomment-507930913, or mute the thread https://github.com/notifications/unsubscribe-auth/AB7NQ2P44GTIT7XG77LXQA3P5QRSFANCNFSM4HODB6GA.

jedbrown commented 5 years ago

Hmm, can you try this? pip install --upgrade --user --trusted-host pypi.org --trusted-host files.pythonhosted.org pip setuptools

akelbert commented 5 years ago

akelbert$ pip install --upgrade --user --trusted-host pypi.org --trusted-host files.pythonhosted.org pip setuptools Could not fetch URL https://pypi.python.org/simple/pip/: There was a problem confirming the ssl certificate: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:749) - skipping Requirement already up-to-date: pip in /Users/akelbert/Developer/local/miniconda3/lib/python3.6/site-packages Could not fetch URL https://pypi.python.org/simple/setuptools/: There was a problem confirming the ssl certificate: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:749) - skipping Requirement already up-to-date: setuptools in /Users/akelbert/Developer/local/miniconda3/lib/python3.6/site-packages

:)

On 7/2/19 10:30 PM, Jed Brown wrote:

Hmm, can you try this? |pip install --upgrade --user --trusted-host pypi.org --trusted-host files.pythonhosted.org pip setuptools|

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1463?email_source=notifications&email_token=AB7NQ2JQKGLY7P55774R2KDP5QTPZA5CNFSM4HODB6GKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZDHIFQ#issuecomment-507933718, or mute the thread https://github.com/notifications/unsubscribe-auth/AB7NQ2MAMJXEG67LL75RU7DP5QTPZANCNFSM4HODB6GA.

prisae commented 5 years ago

Yes, emg3d requires python 3.7 at least.

As it is a new library I decided to use the newest python to leverage some of the new tools in python.

I'll be in the office in an hour or so and will reply to the questions raised in the last few comments.

On 3 July 2019 06:18:59 CEST, Anna Kelbert notifications@github.com wrote:

akelbert$ python3 -m pip --version> pip 9.0.1 from > /Users/akelbert/Developer/local/miniconda3/lib/python3.6/site-packages

(python 3.6)>

akelbert$ pip install --trusted-host pypi.org --trusted-host > files.pythonhosted.org emg3d> Collecting emg3d>   Could not fetch URL https://pypi.python.org/simple/emg3d/: There was

a problem confirming the ssl certificate: [SSL: > CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:749) - skipping>   Could not find a version that satisfies the requirement emg3d (from > versions: )> No matching distribution found for emg3d>

akelbert$ pip install --trusted-host pypi.org --trusted-host > files.pythonhosted.org pip setuptools> Requirement already satisfied: pip in > /Users/akelbert/Developer/local/miniconda3/lib/python3.6/site-packages> Requirement already satisfied: setuptools in > /Users/akelbert/Developer/local/miniconda3/lib/python3.6/site-packages>

Somehow I've never had luck with python :(>

On 7/2/19 10:14 PM, Jed Brown wrote:>

For our reference, can you paste the output from |python3 -m pip > --version|?> I think this answer https://stackoverflow.com/a/29751768/33208 will

resolve the cert issue.>

—> You are receiving this because you were mentioned.> Reply to this email directly, view it on GitHub >

https://github.com/openjournals/joss-reviews/issues/1463?email_source=notifications&email_token=AB7NQ2PN5TT5WUIMYMH5Y4DP5QRSFA5CNFSM4HODB6GKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZDGSII#issuecomment-507930913,

or mute the thread >

https://github.com/notifications/unsubscribe-auth/AB7NQ2P44GTIT7XG77LXQA3P5QRSFANCNFSM4HODB6GA.>

-- > You are receiving this because you were mentioned.> Reply to this email directly or view it on GitHub:> https://github.com/openjournals/joss-reviews/issues/1463#issuecomment-507931659

prisae commented 5 years ago

@akelbert what OS are you on?