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

Branch onno #127

Merged
merged 10 commits into from
Jul 26, 2019
Merged

Branch onno #127

merged 10 commits into from
Jul 26, 2019

Conversation

OnnoEbbens
Copy link
Member

I changed the 'add_recharge' method in project.py and added 'solve_models' and 'add_models' so you can apply these methods to all models at once instead of one model at the time. Changes:

  • 'add_recharge' now has the option to add recharge to multiple models at once instead of just a single model.
  • 'add_models' was added to add multiple models from the available oseries
  • 'solve_models' was added to solve all models in the project at once with an option to ignore solve errors.

Besides changed default parameter bounds of pmin and pmax for rfunc One to -100 and +100 when up is None.

@codecov
Copy link

codecov bot commented Jul 25, 2019

Codecov Report

Merging #127 into dev will decrease coverage by 0.5%.
The diff coverage is 9.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #127      +/-   ##
==========================================
- Coverage   66.05%   65.54%   -0.51%     
==========================================
  Files          31       31              
  Lines        3246     3274      +28     
==========================================
+ Hits         2144     2146       +2     
- Misses       1102     1128      +26
Impacted Files Coverage Δ
pastas/project/project.py 40.41% <9.25%> (-4.4%) ⬇️

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 77cbc01...08d41f5. Read the comment docs.

@raoulcollenteur
Copy link
Member

raoulcollenteur commented Jul 25, 2019

Hi Onno, thanks for your PR with some nice �additions! Couple of small things to do before we can merge:

Add_models method

  • Use the add_model code in the for-loop, now there is twice the code.

add_recharge method

  • Move the method up after the other add methods

All methods

  • Apply PEP8 formatting everywhere

  • Remove warning dependency

  • Should we use mls instead of ml_list?

  • Does it actually make sense to use the Model object or should we move to model name?

Final note on the other changes:

Besides changed default parameter bounds of pmin and pmax for rfunc One to -100 and +100 when up is None.

Let's do this in a separate PR request.

@raoulcollenteur
Copy link
Member

I changed a couple of things and did some commits to the PR. You'll have to pull first. Can you check if it all still works?

What do you think about using model names instead of using model objects?

@OnnoEbbens
Copy link
Member Author

OnnoEbbens commented Jul 25, 2019

Thanks for checking and the changes you made.

  • I changed ml_list to mls

  • I also agree with using model names instead of model objects in the add_models, add_recharge and solve_models methods.

  • I choose to return model objects for the add_models method because add_model also returns a model object. Looking closer at the other add_.. methods I see that add_model is the only one returning something. That makes me wonder is it not a better idea to remove the return statement from the add_model and add_models methods? I did not add this to the PR because it will break backward compatibility.

@raoulcollenteur raoulcollenteur merged commit 12e13b1 into dev Jul 26, 2019
@raoulcollenteur raoulcollenteur deleted the branch_Onno branch July 26, 2019 08:31
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

2 participants