-
Notifications
You must be signed in to change notification settings - Fork 33
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
Joss paper #149
Merged
Merged
Joss paper #149
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
to ensure the Back-compatibility with legacy code, the Appliance method of User class is present. However the method is not further maintained since the nex-gen version of the sofware. To warn the users to move towards the new method, a DeprecationWarning is added.
Issue: it was not easy to find that there were extra dependencies for testing Solution: mention it explicitely in the CONTRIBUTING.md
issue: the test was failing randomly when accessing whether the sampled coincidence were following a normal distribution, due to the fact we apply math.ceil to the random.gauss to get integer number of appliances to be switched on simultaneously solution: get an experimental probability density function and fit a normal distribution to it, look how large is the error of mean and std as a proxy of visual inspection of the graph
issue: using the excel file generated by the "Using tabular inputs to build a model" example one could not run successfully `ramp -i example_excel_usecase.xlsx -n 10` solution: pass the argument flat=False to the Usecase method generate_daily_load_profiles. The postprocessing should actually make use of the newest capabilities
issue: the user could not simulate more than one year because of a requirement that the number of power values for an appliance should not exceed 366 solution: the checks were moved to a function of Appliance class which is called when the usecase is initialized. If the Appliance instance has constant power, its power timeseries is adjusted and a warning is issued. However if the power timeseries was provided by user and does not contain enough values to cover the simulation range, an error is raised.
Also provide a test of the CLI with example file
Remove typo in a string
Add a section about issues
within the current template, 'basicstrap', the navigation on the left sidebar (with the partial TOC) is a bit counter-intuitive. The new template provides a more intituitive sidebars.
Add a line at end of section with link to download the notebook
issue: Python 3.8 is mentioned in the README as the prefered python version for the installation. However, this python vesion is going to be outdated by October 2024. Current RAMP dev can be safely used by python 3.10. Changes: - README.rst --> updateing the installation guide - environment.yml --> updating the python dependency for installing environment through yml file - setup.py --> updating the python_requires accordingly
issue: When a user object is empty cannot be printed due as the __repr__ method uses the save() method to print the properties of the users, which raise Exception in case no appliances are added to the user. solution: in case save() methods returns an exception, the function prints the user_name, and user_num with a message to mention no appliances are assigned to the user object yet.
The comparisons were not saved so the return value was always "True"
As the contribution guidelines have a dedicated in the documentation we replace the link within the readme which was pointing towards the file CONTRIBUTING.md on github with a link which points towards the dedicated contribution guidelines within the documentation
Pre-commit already getting rid of trailing spaces
And fix trailing spaces (pre-commit hooks)
The author was master student from this institution working at RLI at the time and he is researcher at this institution now
FLomb
approved these changes
Jun 4, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR; the proposed merging looks good, and all checks passed, so I approve it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request contains all commits that were made during the review process openjournals/joss-reviews#6418
The changelog will be updated in the release branch