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

Feature/init measures base #564

Merged
merged 9 commits into from
Oct 28, 2022
Merged

Feature/init measures base #564

merged 9 commits into from
Oct 28, 2022

Conversation

tovogt
Copy link
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
11 tasks
@tovogt
Copy link
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
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
Collaborator Author

tovogt commented Oct 28, 2022

I went through everything, it's fine 👍

@chahank
Copy link
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants