Ref/remote download #598

lheagy · 2017-05-04T06:06:13Z

modify the remoteDownload utils so that more of the string handling is done by the util as per #596

Before

basePath = os.path.sep.join(os.path.abspath(os.getenv('HOME')).split(os.path.sep)+['Downloads']+['SimPEGtemp'])
basePath = os.path.abspath(remoteDownload(url, cloudfiles, basePath=basePath+os.path.sep))

After

basePath = download(url, path='~/Downloads', overwrite=True)

I have also updated the examples and tests that rely on this function

lheagy · 2017-05-04T06:06:33Z

SimPEG/EM/Base.py

@@ -256,7 +256,9 @@ def MeSigmaI(self):
        Inverse of the edge inner product matrix for \\(\\sigma\\).
        """
        if getattr(self, '_MeSigmaI', None) is None:
-            self._MeSigmaI = self.mesh.getEdgeInnerProduct(self.sigma, invMat=True)
+            self._MeSigmaI = self.mesh.getEdgeInnerProduct(


this is just white-space

lheagy · 2017-05-04T06:08:15Z

SimPEG/Utils/io_utils.py

    """

    # Download from cloud
    import urllib
    import shutil
    import os
    import sys
+
+    def rename_path(downloadpath):


if we start to make heavy use of this function, I can see a number of scenarios where we do not necessarily want to over-write the contents of SimPEGtemp, so here it will add SimPEGtemp(1) (or up the integer value until we hit a unique name, similar to what you would see with a web browser)

lheagy · 2017-05-04T06:08:52Z

SimPEG/Utils/io_utils.py

@@ -146,35 +147,72 @@ def surface2inds(vrtx, trgl, mesh, boundaries=True, internal=True):
    return insideGrid


-def remoteDownload(url, remoteFiles, basePath=None):
+def remoteDownload(
+    url, remoteFiles, path='.', directory='SimPEGtemp', rm_previous=False


see what you think of these input names @fourndo, @rowanc1

rowanc1

Minor suggestions to the api.

rowanc1 · 2017-05-04T14:24:21Z

SimPEG/Utils/io_utils.py

-    print("Download files from URL...")
+    # ensure we are working with absolute paths
+    path = os.path.abspath(path)
+    downloadpath = os.path.sep.join([path]+[directory])


[path, directory]? Same difference.

Yeah, this is likely confusing. path is the path to your working directory, directory is the name of the new folder we are adding where we download things.

Thoughts on alternatives?

rowanc1 · 2017-05-04T14:33:21Z

examples/04-grav/plot_laguna_del_maule_inversion.py

+        'LdM_topo.topo', 'LdM_input_file.inp'
+    ]
+
+    # Download to Downloads/SimPEGtemp


Here we are not specifying where things get put? I think they should just go directly into pwd? Unless you specify a folder or something?

Can we just rename this to download? seems a bit redundant.

file_names = download([url1, url2], folder='~/Downloads/mag_stuff', overwrite=True) # or file_name = download(url1) # where assert isinstance(file_names, list) assert len(file_names) == 2 assert isinstance(file_name, str)

I think that the default should the the pwd of where the python file is run from, appending a SimPEGtemp seems odd to me. Downloads are downloads.

This also would mean that you don't have to reconstruct the file name after the fact, you just loop through the list:

with open(file_name, 'r') as f: f.read() with open(file_names[0], 'r') as f2: f2.read()

download works for me. @fourndo ?

SimPEGtemp was mainly motivated because we make heavy use of this in examples, so for someone unfamiliar it is easy to find (we can still do this and specify it in the example script rather than the util). However, I agree that can be cleared up by specifying the folder (I am tempted to call it path or directory instead as it is not just a name of a folder but does include path information)

I think that it could be both? You can call abspath on the thing and it should expand it out regardless.

folder='here' folder='or/here' folder='/Users/rowan/downloads/or/even/here'

I see all of these as folders?

ok, fair enough, folder can work

rowanc1 · 2017-05-04T14:36:48Z

Does this work on windows as well?

C:\Users\Myname\Downloads?

lheagy · 2017-05-04T14:41:51Z

This ~ as a path does not work on windows, but os.getenv('HOME') seems to work for most systems.

rowanc1 · 2017-05-04T14:57:57Z

You can use expanduser:
https://docs.python.org/2/library/os.path.html#os.path.expanduser

lheagy · 2017-05-04T15:48:06Z

I think most of this works - the main subtlety here is the behaviour of overwrite. Previously it deleted the old directory and replaced it with a new one, now, we are not forcing the creation of a new directory, so I am hesitant to have overwrite remove the directory - it should only overwrite the files (otherwise I can just see someone trying and rapidly regretting download(url, '~', overwrite=True))

rowanc1 · 2017-05-04T15:58:57Z

Yeah, I think that removing things should never happen. :) Only overwriting files, never folders.

…tories, update examples and tests accordingly

codecov-io · 2017-05-04T19:34:40Z

Codecov Report

Merging #598 into dev will decrease coverage by 0.08%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##              dev     #598      +/-   ##
==========================================
- Coverage   74.75%   74.66%   -0.09%     
==========================================
  Files         106      106              
  Lines       13545    13572      +27     
==========================================
+ Hits        10126    10134       +8     
- Misses       3419     3438      +19

Impacted Files	Coverage Δ
SimPEG/EM/Base.py	`80.23% <100%> (ø)`	⬆️
SimPEG/PF/MagneticsDriver.py	`77.83% <100%> (ø)`	⬆️
SimPEG/Utils/__init__.py	`100% <100%> (ø)`	⬆️
SimPEG/Utils/io_utils.py	`27.58% <48.71%> (-0.2%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bdd221...2818916. Read the comment docs.

lheagy · 2017-05-04T19:41:50Z

please take a look when you get a chance and let me know what you think. Thanks!

rowanc1 · 2017-05-04T19:54:00Z

tests/base/test_utils.py

+        # where
+        assert isinstance(file_names, list)
+        assert len(file_names) == 2
+        assert isinstance(file_name, str)


Ha, it's like I coded...

rowanc1 · 2017-05-04T19:54:21Z

This looks great! thanks @lheagy :)

lheagy added 2 commits May 3, 2017 15:50

update remote download to handle more of the path wrangling as per #596

97e4488

fix filepaths in casing example

102d76f

lheagy requested review from rowanc1 and fourndo May 4, 2017 06:06

lheagy commented May 4, 2017

View reviewed changes

rowanc1 reviewed May 4, 2017

View reviewed changes

rename remoteDownload to download, only overwrite files not direc…

e5c149f

…tories, update examples and tests accordingly

add cleanup test of downloads

9d4172e

lheagy added the enhancement label May 4, 2017

rowanc1 reviewed May 4, 2017

View reviewed changes

lheagy added 3 commits May 4, 2017 16:50

fix tests and examples that I missed in the first go-around

88348c2

clean up how url is printed so that it is clickable in the notebook

cc27951

path --> folder

2818916

lheagy merged commit f7bf464 into dev May 5, 2017

lheagy deleted the ref/remoteDownload branch May 5, 2017 22:41

lheagy mentioned this pull request May 6, 2017

remote download default #596

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ref/remote download #598

Ref/remote download #598

lheagy commented May 4, 2017 •

edited

lheagy May 4, 2017

lheagy May 4, 2017

lheagy May 4, 2017

rowanc1 left a comment

rowanc1 May 4, 2017

lheagy May 4, 2017

rowanc1 May 4, 2017

rowanc1 May 4, 2017

lheagy May 4, 2017

rowanc1 May 4, 2017

lheagy May 5, 2017

rowanc1 commented May 4, 2017

lheagy commented May 4, 2017

rowanc1 commented May 4, 2017

lheagy commented May 4, 2017 •

edited

rowanc1 commented May 4, 2017

codecov-io commented May 4, 2017 •

edited

lheagy commented May 4, 2017 •

edited by fourndo

rowanc1 May 4, 2017

rowanc1 commented May 4, 2017

Ref/remote download #598

Ref/remote download #598

Conversation

lheagy commented May 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rowanc1 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rowanc1 commented May 4, 2017

lheagy commented May 4, 2017

rowanc1 commented May 4, 2017

lheagy commented May 4, 2017 • edited

rowanc1 commented May 4, 2017

codecov-io commented May 4, 2017 • edited

Codecov Report

lheagy commented May 4, 2017 • edited by fourndo

Choose a reason for hiding this comment

rowanc1 commented May 4, 2017

lheagy commented May 4, 2017 •

edited

lheagy commented May 4, 2017 •

edited

codecov-io commented May 4, 2017 •

edited

lheagy commented May 4, 2017 •

edited by fourndo