Skip to content

Feature/init measures base#564

Merged
tovogt merged 9 commits intodevelopfrom
feature/init_measures_base
Oct 28, 2022
Merged

Feature/init measures base#564
tovogt merged 9 commits intodevelopfrom
feature/init_measures_base

Conversation

@tovogt
Copy link
Copy Markdown
Collaborator

@tovogt tovogt commented Oct 26, 2022

Changes proposed in this PR:

  • Refactor the constructor of the Measure class.
  • Properly use the refactored init in all tutorials and other parts of the CLIMADA core code.

PR Author Checklist

PR Reviewer Checklist

@tovogt tovogt mentioned this pull request Oct 26, 2022
Merged
11 tasks
chahank
chahank reviewed Oct 28, 2022
View reviewed changes
chahank
chahank reviewed Oct 28, 2022
View reviewed changes
chahank
chahank reviewed Oct 28, 2022
View reviewed changes
chahank
chahank reviewed Oct 28, 2022
View reviewed changes
chahank
chahank reviewed Oct 28, 2022
View reviewed changes
@tovogt
Copy link
Copy Markdown
Collaborator Author

tovogt commented Oct 28, 2022

Thanks for the review! All of your suggestions were good and I just merged them. Good point about the MeasureSet.from_excel part where attributes that are missing in the file are replaced by default values: We should indeed leave the falling back to the __init__ instead of doing it manually. I adapted the code accordingly in c6141ed

@chahank
Copy link
Copy Markdown
Member

chahank commented Oct 28, 2022

Great! Thanks. I think I did not catch all the instance where defaults where not let to the init. Maybe in measures which is intimately related to the measureset. Shall I go through or have you already checked there too?

@tovogt
Copy link
Copy Markdown
Collaborator Author

tovogt commented Oct 28, 2022

I went through everything, it's fine 👍

@chahank
Copy link
Copy Markdown
Member

chahank commented Oct 28, 2022

Then it is good for me!

@tovogt tovogt merged commit 0a7caf9 into develop Oct 28, 2022
@emanuel-schmid emanuel-schmid deleted the feature/init_measures_base branch October 28, 2022 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chahank chahank chahank left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tovogt @chahank