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

improve solve_models() with tmin/tmax options #144

Merged
merged 2 commits into from
Aug 28, 2019
Merged

improve solve_models() with tmin/tmax options #144

merged 2 commits into from
Aug 28, 2019

Conversation

dbrakenhoff
Copy link
Member

No description provided.

@dbrakenhoff
Copy link
Member Author

implements partial solution for question in #141.

@codecov
Copy link

codecov bot commented Aug 28, 2019

Codecov Report

Merging #144 into dev will decrease coverage by 0.2%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #144      +/-   ##
==========================================
- Coverage   67.35%   67.15%   -0.21%     
==========================================
  Files          31       31              
  Lines        3345     3355      +10     
==========================================
  Hits         2253     2253              
- Misses       1092     1102      +10
Impacted Files Coverage Δ
pastas/project/project.py 39.28% <0%> (-1.63%) ⬇️

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 0e9ad18...1ad658f. Read the comment docs.

Copy link
Member

@OnnoEbbens OnnoEbbens left a comment

Choose a reason for hiding this comment

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

nice

Copy link
Member

@raoulcollenteur raoulcollenteur left a comment

Choose a reason for hiding this comment

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

Nice indeed. I think this is a good way of implementing it. Could be made more general for all arguments passen on to ml.solve in the future.

@raoulcollenteur raoulcollenteur added the enhancement Indicates improvement of existing features label Aug 28, 2019
@raoulcollenteur raoulcollenteur added this to the 0.12.0 milestone Aug 28, 2019
@dbrakenhoff dbrakenhoff merged commit b317e5d into dev Aug 28, 2019
@Hugovdberg
Copy link
Contributor

This could in fact be made more general quite easily by checking the kwargs for pandas.Series objects.

m_kwargs = {}
for key, value in kwargs.items():
    if isinstance(value, pd.Series):
        m_kwargs[key] = value.loc[ml_name]
    else:
        m_kwargs[key] = value
# Convert timestamps
for tstamp in ['tmin', 'tmax']:
    if tstamp in m_kwargs:
        m_kwargs[tstamp] = pd.Timestamp(m_kwargs[tstamp])

[...]
ml.solve(report=report, **m_kwargs)

Also, is there any reason the new arguments are not forced to be passed as keywords (PEP 3102) since Python 3 is required anyway?

@dbrakenhoff
Copy link
Member Author

Good suggestion! We also realized this was something we should eventually make more general, but since I only needed tmin/tmax at the moment, I got lazy.

Feel free to submit a PR with this change, otherwise I'll implement your suggestion sometime later this week.

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

Successfully merging this pull request may close these issues.

None yet

4 participants