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

Joss paper #149

Merged
merged 97 commits into from
Jun 4, 2024
Merged

Joss paper #149

merged 97 commits into from
Jun 4, 2024

Conversation

Bachibouzouk
Copy link
Collaborator

@Bachibouzouk Bachibouzouk commented Jun 3, 2024

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

FLomb and others added 30 commits June 3, 2024 20:45
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
@Bachibouzouk Bachibouzouk marked this pull request as ready for review June 3, 2024 19:39
@Bachibouzouk Bachibouzouk mentioned this pull request Jun 3, 2024
3 tasks
Copy link
Contributor

@FLomb FLomb left a 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.

@Bachibouzouk Bachibouzouk merged commit c0423c0 into development Jun 4, 2024
5 checks passed
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.

5 participants