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

Add WellModel #129

Merged
merged 30 commits into from
Aug 1, 2019
Merged

Add WellModel #129

merged 30 commits into from
Aug 1, 2019

Conversation

dbrakenhoff
Copy link
Member

Add WellModel to pastas:

@dbrakenhoff dbrakenhoff added the development Indicates development of new features label Jul 29, 2019
@dbrakenhoff dbrakenhoff added this to the 0.12.0 milestone Jul 29, 2019
@codecov
Copy link

codecov bot commented Jul 29, 2019

Codecov Report

Merging #129 into dev will increase coverage by 0.12%.
The diff coverage is 85.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #129      +/-   ##
==========================================
+ Coverage   65.52%   65.65%   +0.12%     
==========================================
  Files          31       31              
  Lines        3275     3296      +21     
==========================================
+ Hits         2146     2164      +18     
- Misses       1129     1132       +3
Impacted Files Coverage Δ
pastas/rfunc.py 80.51% <100%> (+0.25%) ⬆️
pastas/model.py 80.88% <100%> (+0.15%) ⬆️
pastas/timeseries.py 76.87% <40%> (+0.07%) ⬆️
pastas/stressmodels.py 75.52% <87.5%> (-0.02%) ⬇️

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 0c1b820...56f4cf1. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 29, 2019

Codecov Report

Merging #129 into dev will increase coverage by 0.07%.
The diff coverage is 84.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #129      +/-   ##
==========================================
+ Coverage   65.52%   65.59%   +0.07%     
==========================================
  Files          31       31              
  Lines        3275     3375     +100     
==========================================
+ Hits         2146     2214      +68     
- Misses       1129     1161      +32
Impacted Files Coverage Δ
pastas/model.py 81.13% <100%> (+0.4%) ⬆️
pastas/timeseries.py 76.87% <40%> (+0.07%) ⬆️
pastas/stressmodels.py 74.78% <75%> (-0.77%) ⬇️
pastas/rfunc.py 82.28% <97.56%> (+2.02%) ⬆️
pastas/solver.py 45.22% <0%> (-3.25%) ⬇️

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 0c1b820...aa1bb0d. Read the comment docs.

@raoulcollenteur
Copy link
Member

I will look at the code later today and probably merge the PR (just had a quick look and it al looks fine). I just need to read the issues #201 and #108 a bit and compare to the formulas we used in the Pastas paper and check if the notebook still works.

Do you have example notebook to go with this PR? That would be a nice addition :)

@dbrakenhoff
Copy link
Member Author

I modified the existing example_Wellmodel.py to show how it works. I think I can easily convert that into a notebook if that is preferred over a Python script.

pastas/model.py Outdated Show resolved Hide resolved
@raoulcollenteur
Copy link
Member

raoulcollenteur commented Jul 31, 2019

As I was afraid this PR changes the result of the example 2 of the groundwater paper, which is not something we want. Most likely this is caused by the scaling of the step response.

Maybe we best add a new response function to rfunc.py?

EDIT: I'll read a bit through #108 tomorrow and put some more thought into this.

@dbrakenhoff
Copy link
Member Author

Do you think the boundaries of the parameter or the different value of A are impacting the optimization of A? Because otherwise there is no reason for the result to change... I only moved a factor that wasn't contained in A into the parameter.

I can't solve the example from the paper with the LmFitSolver because of the covar error you mentioned elsewhere in #133. But I do get a fairly similar result when solving with LeastSquares and multiplying the parameters and stderr by the factor 2*k0(rho). Left is my result, and right is the old result.

image

Having said all that, I do think a new rfunc is a good idea though, that way all the weirdness caused by the WellModel can be contained in a separate class. I think that's clearest from the user perspective. It can inherit from Hantush and only needs a extra few lines of code. If you agree, I'll set it up.

- remove changes related to WellModel from Hantush
- add new rfunc HantushWellModel
- change params to A, lab, cS
- set initial, pmin, pmax for lab to 100, 1, 1e6
- set gain() method to calculate the A Hantush returns now
- set meanstress to the max of the std of the stresses
…ameters option to get parameters with distance
@dbrakenhoff
Copy link
Member Author

As you can see I went ahead with your suggestion for a new rfunc class. It's better this way, as the differences between Hantush and HantushWellModel are now more obvious (instead of being hidden in if-statements in the code).

@raoulcollenteur
Copy link
Member

Nice! I'll check it now and hopefully merge soon :)

@raoulcollenteur
Copy link
Member

Okay all looks good and ready to merge! The notebook looks very good now, really like the stack_plot. This could become a nice new plotting method acutally! Where all contribution are stacked. I think at some point we might want to provide the stresses from a csv-file and move away from the Menyanthes file but that can be done later. One small question before merging:

  • What is the use/advantage of the sort_wells argument?

@dbrakenhoff
Copy link
Member Author

Theres no real need to sort the wells, I just wanted a nice list ordered by distance for myself and I figured others might too, so i added it as an option. I have a few models with quite a few wells in the WellModel and then it help keep things a little more organized if they're sorted by distance.

@raoulcollenteur
Copy link
Member

raoulcollenteur commented Aug 1, 2019

Theres no real need to sort the wells, I just wanted a nice list ordered by distance for myself and I figured others might too, so i added it as an option. I have a few models with quite a few wells in the WellModel and then it help keep things a little more organized if they're sorted by distance.

Okay makes sense. I added the keyword to the dump method. In general all arguments used in the init method of a stressmodel need to be written in the dump method :)

Merging now. Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Indicates development of new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants