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.

Sign up for GitHub

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

Jump to bottom

new splitting function #395

Merged
merged 30 commits into from Sep 6, 2022
Merged

new splitting function #395

merged 30 commits into from Sep 6, 2022

Conversation

mariamanoli
Copy link
Collaborator

This is a pull request for the new energy splitting model DAZLS. The current branch contains two new files that need to be reviewed: better_dazls.py and better_dazls.ipynb

mariamanoli and others added 6 commits August 4, 2022 10:01
@mariamanoli
code cleaning tasks
a8b8826
@mariamanoli
improves vol2
a31fee4
@mariamanoli
improves dazls
e80f382 
fit, prediction and score functions
@mariamanoli
Update better_dazls.py
750db07 
add text in the functions
@mariamanoli
correct problems before pull request
d317ebb
@mariamanoli
DAZLS notebook
22e509b 
This notebook includes the initial DAZLS model and the improved version of the DAZLS model.
@actions-user
Format Python code with Black
e59aca0 
Signed-off-by: black <action@github.com>
FrankKr
FrankKr requested changes Aug 10, 2022
View reviewed changes
Copy link
Collaborator

@FrankKr FrankKr left a comment

Choose a reason for hiding this comment

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

Nice job combining all the stuff!
Suggestions:

  • Move the better_dazls.ipynb to icarus-analyses
  • Move all the csv files to icarus-analyses
  • Run to notebook to train a model on your laptop, store that model, and add the stored model to somewhere in openstef

openstef/tasks/split_forecast.py Outdated Show resolved Hide resolved
openstef/model/regressors/better_dazls.py Outdated Show resolved Hide resolved
openstef/model/regressors/better_dazls.py Outdated Show resolved Hide resolved
DeniseMeerkerk
DeniseMeerkerk reviewed Aug 10, 2022
View reviewed changes
Copy link
Collaborator

@DeniseMeerkerk DeniseMeerkerk left a comment

Choose a reason for hiding this comment

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

Very nice job Maria!

JanMaartenvanDoorn
JanMaartenvanDoorn reviewed Aug 22, 2022
View reviewed changes
Copy link
Collaborator

@JanMaartenvanDoorn JanMaartenvanDoorn left a comment •
edited

Choose a reason for hiding this comment

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

Nice addition to the code! Added some remarks to improve clarity
In addition it is also good to remove all the code related to the old splitting method. We do not need the code to determine the splitting coefficients anymore.

openstef/pipeline/create_component_forecast.py Outdated Show resolved Hide resolved
openstef/pipeline/create_component_forecast.py Show resolved Hide resolved
openstef/model/regressors/better_dazls.py Outdated Show resolved Hide resolved
openstef/model/regressors/better_dazls.py Outdated Show resolved Hide resolved
openstef/model/regressors/better_dazls.py Outdated Show resolved Hide resolved
openstef/model/regressors/better_dazls.py Outdated Show resolved Hide resolved
openstef/model/regressors/better_dazls.py Outdated Show resolved Hide resolved
openstef/model/regressors/better_dazls.py Outdated Show resolved Hide resolved
@mariamanoli
corrections after review
490de4f
@mariamanoli
delete html files
e459cf9
@mariamanoli
correct github build failures
2a70bd8 
add license, change better_dazls name to dazls, add new .sav file, fix run errors
FrankKr
FrankKr approved these changes Sep 5, 2022
View reviewed changes
Copy link
Collaborator

@FrankKr FrankKr left a comment

Choose a reason for hiding this comment

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

Looks really clear!
I made some minor remarks on naming conventions.

openstef/model/regressors/dazls.py Outdated Show resolved Hide resolved
openstef/model/regressors/dazls.py Show resolved Hide resolved
openstef/model/regressors/dazls.py Outdated Show resolved Hide resolved
openstef/model/regressors/dazls.py Outdated Show resolved Hide resolved
openstef/model/regressors/dazls.py Outdated Show resolved Hide resolved
openstef/model/regressors/dazls.py Outdated Show resolved Hide resolved
openstef/model/regressors/dazls.py Outdated Show resolved Hide resolved
openstef/model/regressors/dazls.py Outdated Show resolved Hide resolved
MartijnCa
MartijnCa approved these changes Sep 5, 2022
View reviewed changes
Copy link
Collaborator

@MartijnCa MartijnCa left a comment

Choose a reason for hiding this comment

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

Looks good Maria! I only posted one comment for a little bit of clarification.

mariamanoli and others added 6 commits September 6, 2022 10:16
@mariamanoli
naming changes
0d1d3f5
@actions-user
Format Python code with Black
c52128c 
Signed-off-by: black <action@github.com>
@mariamanoli
re-run dazls_stored notebook to load and store the new DAZLS model
1b10aca
@mariamanoli
Merge branch 'splitting_improves_vol1' of https://github.com/OpenSTEF…
b378906 
…/openstef into splitting_improves_vol1
@JanMaartenvanDoorn
Update setup.py
6b00097 
bumped version number

Signed-off-by: Jan Maarten van Doorn <52956303+JanMaartenvanDoorn@users.noreply.github.com>
@JanMaartenvanDoorn
Merge branch 'main' into splitting_improves_vol1
b3e95cc
@JanMaartenvanDoorn JanMaartenvanDoorn merged commit 887e556 into main Sep 6, 2022
@JanMaartenvanDoorn JanMaartenvanDoorn deleted the splitting_improves_vol1 branch September 6, 2022 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DeniseMeerkerk DeniseMeerkerk DeniseMeerkerk left review comments

@JanMaartenvanDoorn JanMaartenvanDoorn JanMaartenvanDoorn left review comments

@FrankKr FrankKr FrankKr approved these changes

@MartijnCa MartijnCa MartijnCa approved these changes

@wfstoel wfstoel Awaiting requested review from wfstoel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mariamanoli @FrankKr @DeniseMeerkerk @MartijnCa @JanMaartenvanDoorn @actions-user