Make full use of UseCase class

[Bachibouzouk]

Fix #83
Fix #81
Closes #35
Closes #58

Started to identify where the code need to be changed and written TODOS

First rewrite how to run ramp, then incorporate @mohammadamint plotting scheme into the usecase, user and appliance classes, then modify cli for .xlsx users, then go through the docs and the jupyter notebook examples to adhere to the new unified way of running ramp

[Bachibouzouk]

@mohammadamint @FLomb, I edited the way the input files could be run, do you mind providing a WIP review before I go further into this direction?

[Bachibouzouk]

@mohammadamint, @FLomb - here one can add date_start and or date_end to run simulations. test need to be written for the various possible case (no date_startbut date_end and num_days provided, etc..)

[mohammadamint]

@Bachibouzouk i am off grid with very poor internet connection! Sorry 😐

[Bachibouzouk]

We can provide a better warning here letting the ramp user know about the UseCase class

[Bachibouzouk]

This test help make sure the notebooks in the documentation are not broken. This is a smoke test, a single error raised upon execution of one cell in a notebook is enough to fail the test. Failed notebook are saved in the root of the repo for further inspection (to be deleted then)

[FLomb]

I like how the UseCase allows for the easy use of the old inputs. At first, I didn't get why all the operations on these inputs (plotting, etc.) were included in the input files, but now I get that this was the plan agreed in response to issue #83 as well as something allowing people to run via the command line. So that's good.

On the other hand, the fix to issue #83 does not seem to automatically solve also the very much related issue #81. I am now looking into that so that we may solve both at once with this PR.

[FLomb]

I am a bit confused by why here we say .py and .xlsx file formats are supported for ramp command line, while in the cli.py file we say that only .xlsx is supported for ramp command line.

[Bachibouzouk]

It is because it is a work in progress and I haven't edited things everywhere yet :)

[FLomb]

I tested running the .py example files from the command line, and it works smoothly. Nice job! One minor comment: when the CLI prints the start and end times based on the given number of days, it includes decimal figures for the seconds, which are redundant. For example:

please indicate the number of days to be generated: 7
Please wait...
You will simulate 7 days from 2023-11-29 16:23:05.286940 until 2023-12-05 16:23:05.286940

Can we get rid of the decimal figures for the seconds?

[FLomb]

I like how the UseCase allows for the easy use of the old inputs. At first, I didn't get why all the operations on these inputs (plotting, etc.) were included in the input files, but now I get that this was the plan agreed in response to issue #83 as well as something allowing people to run via the command line. So that's good.

On the other hand, the fix to issue #83 does not seem to automatically solve also the very much related issue #81. I am now looking into that so that we may solve both at once with this PR.

So, it looks like the issue can still be easily fixed the way I suggested in issue #81. It's enough to just remove lines 84 and 85 that erroneously add the ramp prefix to the file path. In fact, adding the ramp prefix turns the input file path it into a ramp sub-module, which only works for the three pre-defined example input files, but which is definitely not what this function should be doing, which is pointing at physical, local files and converting them. Can we just remove those two lines please?

[Bachibouzouk]

So, it looks like the issue can still be easily fixed the way I suggested in issue #81. It's enough to just remove lines 84 and 85 that erroneously add the ramp prefix to the file path. In fact, adding the ramp prefix turns the input file path it into a ramp sub-module, which only works for the three pre-defined example input files, but which is definitely not what this function should be doing, which is pointing at physical, local files and converting them. Can we just remove those two lines please?

I also tested and I agree, I will remove two two lines :)

Right now I am fixing the jupyter notebooks, this is interesting as it raises some questions about RAMP usage like "Should a RAMP user be able to just define an appliance and still generate a profile for this appliance without providing neither a User nor a Usecase", same question for User without Usecase.

I think it should be possible and we should make it as intuitive as possible for the RAMP user to use RAMP, providing some default values when the RAMP user did not provide information. The best would be that I prepare a list of those questions for next RAMP dev meeting so we can all voice our opinion on those questions, what do you think @FLomb @mohammadamint ?

[FLomb]

So, it looks like the issue can still be easily fixed the way I suggested in issue #81. It's enough to just remove lines 84 and 85 that erroneously add the ramp prefix to the file path. In fact, adding the ramp prefix turns the input file path it into a ramp sub-module, which only works for the three pre-defined example input files, but which is definitely not what this function should be doing, which is pointing at physical, local files and converting them. Can we just remove those two lines please?

I also tested and I agree, I will remove two two lines :)

Right now I am fixing the jupyter notebooks, this is interesting as it raises some questions about RAMP usage like "Should a RAMP user be able to just define an appliance and still generate a profile for this appliance without providing neither a User nor a Usecase", same question for User without Usecase.

I think it should be possible and we should make it as intuitive as possible for the RAMP user to use RAMP, providing some default values when the RAMP user did not provide information. The best would be that I prepare a list of those questions for next RAMP dev meeting so we can all voice our opinion on those questions, what do you think @FLomb @mohammadamint ?

Sounds good to discuss this in the next meeting. On the other hand, it would have been nice to wrap up this PR in such a way as to release v0.5.0 and be able to send the paper/code for review to JOSS before the next meeting. But, of course, if the notebooks need fixing anyway for working with the new UseCase, this is something to address before making a release.

[FLomb]

I have updated, fixed (when needed) and refined all the Jupyter notebooks. The problem is that all the files in docs/source/examples/, which are essentially an .rst copy-paste of the notebooks, would also need equivalent updating (actually, even more updating because they are not even up to date with the state the notebooks wee before my changes).

I have already raised some time ago the issue that requiring this kind of double work any time we want to update the notebooks is very inefficient and tedious. Can we somehow automate the Jupyter-notebooks translation into .rst pages?

[FLomb]

A problem I see in the README.rst file is that we import the UseCase but never use it. We just run the profiles for one User, which is not what one typically wants to do. We should show how to add Users to a UseCase and how to run via the UseCase class.

[Bachibouzouk]

Agree, I just translated the existing example but we should also showcase the UseCase more

[FLomb]

The UseCase class now has several new functionalities. Is the api_references.rst document up to date with those? I don't think it is

[Bachibouzouk]

And I also see that older functionalities are present in the docs, there should certainly be a command to automatize the building of API reference from source code. How did you build it inititally @mohammadamint ?

[mohammadamint]

I build them by manually adding the functions in the toctree. As there are some functions that are private, i think we should avoid including all of them, just put the one that the user might interact with

[FLomb]

This is the key example we provide, the one with the basic features. We import the UseCase class and we show it allows to carry out a whole-year analysis, but we don't use it later on for non-whole-year profiles because we only carry out the exercise for one user type (household). Should we add a second user type at the end and showcase how the UseCase is also needed to simulate and aggregate multiple users, regardless of whether we want a whole-year simulation? Otherwise, it seems like UseCase is only something needed to simulate a whole year.

[mohammadamint]

I have updated, fixed (when needed) and refined all the Jupyter notebooks. The problem is that all the files in docs/source/examples/, which are essentially an .rst copy-paste of the notebooks, would also need equivalent updating (actually, even more updating because they are not even up to date with the state the notebooks wee before my changes).

I have already raised some time ago the issue that requiring this kind of double work any time we want to update the notebooks is very inefficient and tedious. Can we somehow automate the Jupyter-notebooks translation into .rst pages?

Rather than implementing all the changes both in rst files and jupyter files, we can download the modified jupyter files as rst to replace the docs examples. But there is a way to automatize this. I will open an issue to work on it.

[mohammadamint]

I have updated, fixed (when needed) and refined all the Jupyter notebooks. The problem is that all the files in docs/source/examples/, which are essentially an .rst copy-paste of the notebooks, would also need equivalent updating (actually, even more updating because they are not even up to date with the state the notebooks wee before my changes).
I have already raised some time ago the issue that requiring this kind of double work any time we want to update the notebooks is very inefficient and tedious. Can we somehow automate the Jupyter-notebooks translation into .rst pages?

Rather than implementing all the changes both in rst files and jupyter files, we can download the modified jupyter files as rst to replace the docs examples. But there is a way to automatize this. I will open an issue to work on it.

@FLomb I could manage this issue with a script. I will create a PR on top of these changes

[FLomb]

@Bachibouzouk I guess you updated these .rst files based on the script developed by @mohammadamint in PR #104, but without adding the script itself, which was yet to be merged?

I just merged PR #104 into development, so it should be ok to merge this into development, too.

[FLomb]

Ok, so I see only two remaining open mini-issues with this PIR:

• The README.rst is not showcasing the UseCase functionality, although it is imported (see my earlier comment)
• The API reference is not up to date, and we don't have it automated, so updating it could be tedious as of now

For the first point, I can take care of updating the README.rst at some point today. For the API reference, I guess nobody will have the time nor the motivation to manually update it; should we remove it altogether and fix this in an automated way in the future?

[mohammadamint]

Ok, so I see only two remaining open mini-issues with this PIR:

• The README.rst is not showcasing the UseCase functionality, although it is imported (see my earlier comment)
• The API reference is not up to date, and we don't have it automated, so updating it could be tedious as of now

For the first point, I can take care of updating the README.rst at some point today. For the API reference, I guess nobody will have the time nor the motivation to manually update it; should we remove it altogether and fix this in an automated way in the future?

@FLomb We just need to add the names of the functions we want to include in this file:
docs/source/api_references.rst

I can take care of this, but I see some new functions also that dont have the docstrings yet.

[mohammadamint]

Ok, so I see only two remaining open mini-issues with this PIR:

• The README.rst is not showcasing the UseCase functionality, although it is imported (see my earlier comment)
• The API reference is not up to date, and we don't have it automated, so updating it could be tedious as of now

For the first point, I can take care of updating the README.rst at some point today. For the API reference, I guess nobody will have the time nor the motivation to manually update it; should we remove it altogether and fix this in an automated way in the future?

@FLomb We just need to add the names of the functions we want to include in this file: docs/source/api_references.rst

I can take care of this, but I see some new functions also that dont have the docstrings yet.

@FLomb I tried to automate the API doc inclusion in the doctree. This is making the docs a little bit messy. I still think just adding the names of the functions we want to include in the API docs, should be the cleanest way to do it. @Bachibouzouk What do you think? I am using autoclass to automatically include all the functions documented.

[Bachibouzouk]

@Bachibouzouk What do you think? I am using autoclass to automatically include all the functions documented.

Yes I agree with this, there is not many methods to document anyways, it is quite quick to do it manually :)

[Bachibouzouk]

I recreated the plot_class rst file from the .ipynb by explicitely removing the cells with plotly figure because the size of the file was 10MB. I replaced them by the png I created manually. As the Plot_class.rst was already in git history I am not sure I changed much the size of the repo and we can also reverse to use directly the .rst with plotly figures embedded (this is heavy because all the data for the figures are present in the html as well as the plotly js code so that the figures are interactive.

[Bachibouzouk]

@FLomb @mohammadamint maybe you can have a look at my latest changes to the documentation and my addendum to the changelog, and also to this comment: #102 (comment)

Then we can finally merge, what is not perfect can be corrected into subsequent PRs, I think this one is already big enough now ^^

[FLomb]

All seems to be in good order now, so I'll approve the PR