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

Ref/tdem testing #620

Merged
merged 51 commits into from Jun 28, 2017
Merged

Ref/tdem testing #620

merged 51 commits into from Jun 28, 2017

Conversation

lheagy
Copy link
Member

@lheagy lheagy commented Jun 24, 2017

  • clean up in the time domain testing
  • efficiency fix in the Problem class. Now we check if the model has truly updated before clearing matrices:
    image

image

if hasattr(self, prop):
delattr(self, prop)
def _on_model_update(self, change):
if self.model is None or not np.allclose(self.model, change['value']):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if an early exit of self.model is change['value'] would be faster. If the model is large a point wise comparison is going to be slow. If they are pointing to the same block in memory, then we can skip that.


# class TDEM_DerivTests(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have these tests perhaps tested on dev-->master? Just so that we know that it is still working, because they are useful for debugging.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right now the TDEM tests are still timing out, so this would first take some re-organizing of tests

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this back on the test_TDEM_DerivAdjoint

@codecov
Copy link

codecov bot commented Jun 24, 2017

Codecov Report

Merging #620 into em/dev will decrease coverage by <.01%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           em/dev     #620      +/-   ##
==========================================
- Coverage   74.33%   74.32%   -0.01%     
==========================================
  Files         105      105              
  Lines       13956    13960       +4     
==========================================
+ Hits        10374    10376       +2     
- Misses       3582     3584       +2
Impacted Files Coverage Δ
SimPEG/Utils/modelutils.py 59.45% <100%> (ø) ⬆️
SimPEG/Problem.py 83.03% <75%> (-0.3%) ⬇️
SimPEG/EM/TDEM/ProblemTDEM.py 93.56% <0%> (-0.23%) ⬇️

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 1927828...60f0f29. Read the comment docs.

if hasattr(self, prop):
delattr(self, prop)
def _on_model_update(self, change):
if self.model is change['value']:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change['prev'] is change['value'] the model will always be the value!!

Copy link
Member

@rowanc1 rowanc1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the if (not this and not that) a bit unclear as to why you would skip the check. If you are checking a corner case, then I would say err on the side of clearing model dependence and just remove this logic?

However, if your check on undefined and None is to make sure that these don't fail the np.allclose then I would suggest changing that to

isinstance(change['previous'], np.ndarray) and 
isinstance(change['value'], np.ndarray) and 
np.allclose(change['previous'], change['value'])

Which is checking what you actually want...!

So something like:

if change['previous'] is change['value']:
    return  # model has not changed
if (
    isinstance(change['previous'], np.ndarray) and 
    isinstance(change['value'], np.ndarray) and 
    np.allclose(change['previous'], change['value'])
):
    return  # model has not *really* changed

I think your current code would break by setting model to properties.undefined, for example.

if change['previous'] is change['value']:
return
if (
not isinstance(change['previous'], properties.utils.Sentinel) and
Copy link
Member

@rowanc1 rowanc1 Jun 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that you can say change['previous'] is not properties.undefined there is only ever one instance of the sentinel hanging around, and it is much more clear what you care looking to do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect, thanks @rowanc1

@@ -1,160 +0,0 @@
from __future__ import division, print_function
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated test_TDEM_DerivAdjoint to test multiple sources, so this test is now redundant

@@ -130,7 +130,8 @@ def test_misfit(self):
self.survey.dpred(m), lambda mx: self.p.Jvec(self.m0, mx)
],
self.m0,
plotIt=False
plotIt=False,
num=3
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sgkang: I fixed the number of iterations this runs. If we let it go too far it fails b/c we hit machine precision.

download([url+f for f in cloudfiles], folder=basePath, overwrite=True)
# directory where the downloaded files are

url = "https://storage.googleapis.com/simpeg/Chile_GRAV_4_Miller/Chile_GRAV_4_Miller.tar.gz"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fourndo, @craigm, I have zipped up the files into a .tar.gz (the originals are still in the same place, but downloading a .tar.gz is much faster than 4 independent files)


url = "https://storage.googleapis.com/simpeg/Chile_GRAV_4_Miller/Chile_GRAV_4_Miller.tar.gz"
downloads = download(url, overwrite=True)
basePath = downloads.split(".")[0]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also downloaded to the current directory (a folder called Chile_GRAV_4_Miller will be created). This makes it easier to work with different systems and to keep track of the files (esp on travis)

@@ -392,7 +392,7 @@ def run(plotIt=True, saveFig=False, cleanup=True):

ax0.semilogx(sigma_re, z, 'k', lw=2, label="RESOLVE")
ax0.semilogx(sigma_sky, z, 'b', lw=2, label="SkyTEM")
ax0.set_ylim(-100, 0)
ax0.set_ylim(-50, 0)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the range we used in the paper

# Download to Downloads/SimPEGtemp
basePath = os.path.expanduser('~/Downloads/simpegtemp')
download([url+f for f in cloudfiles], folder=basePath, overwrite=True)
url = "https://storage.googleapis.com/simpeg/Chile_GRAV_4_Miller/Chile_GRAV_4_Miller.tar.gz"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fourndo are you good with this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm good with that.

@@ -2,7 +2,7 @@
import numpy as np
from scipy.interpolate import griddata, interp1d

def surface2ind_topo(mesh, topo, gridLoc='CC', method='cubic', fill_value=np.nan):
def surface2ind_topo(mesh, topo, gridLoc='CC', method='nearest', fill_value=np.nan):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @fourndo!

@lheagy
Copy link
Member Author

lheagy commented Jun 28, 2017

Thanks for your input all!

@lheagy lheagy merged commit 1aab2bf into em/dev Jun 28, 2017
@lheagy lheagy deleted the ref/tdem-testing branch June 28, 2017 02:03
@lheagy lheagy mentioned this pull request Jun 28, 2017
2 tasks
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.

None yet

5 participants