Plot class for ramp visualization and data analysis routines (isse #63) #89

mohammadamint · 2023-10-15T16:48:36Z

The new features added will allow the user to perform some data analysis and visualization routines:

changes

*. ramp/post_process/post_process.py

the class Plot is the main class carrying on the plotting and data analysis functionalities with custom APIs providing access to some pd.DataFrame features also easy to plot functions conencting to plotly and matplotlibe methods.
different plotting classes that include a specific plot type visualiztion using two different engines (plotly & matplotlib)

*. ramp.init.py

adding the Plot class to the init.

documentation

the functions API docuemntation is added to the API documentaiton
A full example titled "Plot class" is added to the example gallery describing the full potential of the new functionalities

Bachibouzouk · 2023-10-23T10:06:54Z

ramp/post_process/post_process.py

+
+valid_units = ('kW',"W","MW","GW","TW")
+class Run:
+


It would be good to add a docsting to the class so that the people can run help(Run)

To me these methods should be added to the existing Usecase class because here Run is simply a Usecase instance with only one User. If it is integrated into Usecase then the usage I envision for the future is to always take a Usecase class which has a method run and for which one can define datatimeIndexes. The method generate_profiles could be included into Usecase.generate_daily_load_profiles method for example

Bachibouzouk · 2023-10-23T10:09:28Z

ramp/post_process/post_process.py

+            A Plot object
+        """
+
+        if file.split(".")[-1] == "csv":


You can also use file.endswith(".csv")

This is just a side comment

Bachibouzouk · 2023-10-23T10:10:56Z

ramp/post_process/post_process.py

+        elif file.split(".")[-1] == "xlsx":
+
+            df = pd.read_excel(file,sheet_name=sheet_name,index_col=0,header=0)
+


Maybe an error message if the extension is not supported?

Bachibouzouk · 2023-10-23T10:13:41Z

ramp/post_process/post_process.py

+class LinePlot:
+    """Line plot functions for Plotly and matplotlib engine
+    """
+    def plotly(df,**layout):


Suggested change

def plotly(df,**layout):

def plotly(self, df,**layout):

If this is a method class, you should add self, same goes for many occurences

Bachibouzouk · 2023-10-23T10:18:06Z

ramp/post_process/post_process.py

+        """
+        df = self.df.copy()
+
+        df = eval(f"df{conversion}")


Suggested change

df = eval(f"df{conversion}")

df = df * conversion

Using eval can lead to safety issues, it is considered good practice not to use it in code that you publish. As you only use it once, there is certainly a way around using eval we can find :)

Bachibouzouk · 2023-10-23T10:24:16Z

ramp/post_process/post_process.py

+
+
+
+class LinePlot:


You seem to only use LinePlot, AreaPlot and ShadowPlot classes once in each of the corresponding methods of the class Plot. To me it would make the code easier to read/understand if those are discarded and their code simply added inside the line, area and shadow methods of Plot. Especially because most of the logic of lineis inLinePlot`. if there is a bigger plan for future use of such classes let me know, otherwise I would strongly recommand moving the code into the methods

would you keep to different functions for plotly and matplotlib for each type of plot, or simply use an if/else statement inside a plot function for the two options.

the main reason for having separate classes was to encapsulate the two plotting engine for each plot type, mainly for making it more readable.

Bachibouzouk · 2023-10-23T10:28:36Z

ramp/post_process/post_process.py

+        df = df.sort_values(by=column,ascending=False)
+        df.index = [i for i in range(1,len(df.index)+1)]
+
+        return getattr(LinePlot,engine)(df,**kwargs)


Here you could for example use return self.line(df,**kwargs) and make such that line does not take df=self.df it df is recieved as an argument

Bachibouzouk

Hello @mohammadamint , great work!

I made some comments to integrate your work in a more maintainable way, let me know if anything is unclear.

Shouldn't we move the rest of the functions like export_series into Plot class as this function turns anyway the profiles into a DataFrame and uses to_csv method. We should then look for other uses of post_processing function in the examples and fix them, to avoid that people use both ways. I would suggest to keep old ways to not break code but that we replace the code inside the function and use Plot methods inside those "deprecated" functions.

We should maybe discuss live in which order to merge the PRs and rebase your PR on top of the latest development branch if your branch is to be the last one to be merged. Let us contact each other via email to find a moment which fits us both :)

If you don't have time at the moment I could also make a PR onto this PR with my suggested changes.

mohammadamint · 2023-10-23T17:24:10Z

Hello @mohammadamint , great work!

I made some comments to integrate your work in a more maintainable way, let me know if anything is unclear.

Shouldn't we move the rest of the functions like export_series into Plot class as this function turns anyway the profiles into a DataFrame and uses to_csv method. We should then look for other uses of post_processing function in the examples and fix them, to avoid that people use both ways. I would suggest to keep old ways to not break code but that we replace the code inside the function and use Plot methods inside those "deprecated" functions.

We should maybe discuss live in which order to merge the PRs and rebase your PR on top of the latest development branch if your branch is to be the last one to be merged. Let us contact each other via email to find a moment which fits us both :)

If you don't have time at the moment I could also make a PR onto this PR with my suggested changes.

Hi @Bachibouzouk

Thanks for the nice comments. I would like to implement them before merging the PR, as they seem very useful to improve the readability of the code. I can do it in the upcoming days if it is fine!

Regarding the use of the existing functionalities into the Plot class, I absolutely agree with that, I can explore and work on this if needed.

I have quiet an open agenda this week! so we can meet almost everyday based on your availability!

- Improving the Plot class docstring - Improving the error handling with invalid inputs by user regarding the file extensions and plot engine - Improving the readability of the plotting scripts by merging the plot classes into Plot methods

Bachibouzouk · 2023-10-27T08:51:54Z

ramp/post_process/post_process.py

@@ -90,6 +90,9 @@ def export_series(stoch_profiles_series, j=None, fname=None, ofname=None):
    else:
        print("No path to a file was provided to write the results")
 <<<<<<< HEAD
+<<<<<<< HEAD


You have pushed conflicted files here

Bachibouzouk · 2023-10-27T11:02:11Z

@mohammadamint - if you rebased the branch onto development, how come there are still conflicts?

mohammadamint · 2023-10-30T20:56:11Z

@mohammadamint - if you rebased the branch onto development, how come there are still conflicts?

Yes! Indeed. Not sure how that happened, but the conflicts are very easy to solve. if everything else, I can solve the conflict and merge it.

Bachibouzouk · 2023-10-31T08:34:43Z

Yes! Indeed. Not sure how that happened, but the conflicts are very easy to solve. if everything else, I can solve the conflict and merge it.

What is wierd is that it shows 25 commits, whereas I remember there where two, rebasing should keep the number of commits constant

Bachibouzouk · 2023-10-31T08:43:10Z

@mohammadamint - I did rebase locally on a copy of ramp_visualization with git rebase -i development and I only needed to fix one easy conflict in the ramp/post_process/post_process.py, the result is 2 commits on top of development did you pull the latest version of development locally before you rebased?

mohammadamint · 2023-10-31T10:17:50Z

@mohammadamint - I did rebase locally on a copy of ramp_visualization with git rebase -i development and I only needed to fix one easy conflict in the ramp/post_process/post_process.py, the result is 2 commits on top of development did you pull the latest version of development locally before you rebased?

Yes I did. but, I am not able to fix the conflicts that arose after rebasing properly. Would creating a new branch from the latest development work?

Bachibouzouk · 2023-10-31T13:31:50Z

Would creating a new branch from the latest development work?

I am not sure what you did, did you force push the ramp_visualisation rebased branch ? I can also force push mine, but then my name will be intertwined in the commit (still leaving you as the author though

mohammadamint · 2023-10-31T15:43:30Z

Would creating a new branch from the latest development work?

I am not sure what you did, did you force push the ramp_visualisation rebased branch ? I can also force push mine, but then my name will be intertwined in the commit (still leaving you as the author though

As soon as the solution works, it doesn't matter :-) .

mohammadamint · 2023-10-31T19:18:15Z

Is the conflict still there? @Bachibouzouk

Bachibouzouk · 2023-10-31T20:07:22Z

Is the conflict still there? @Bachibouzouk

Yes I reforced push your branch because I had to prepare dinner and I will tackle this now

- Improving the Plot class docstring - Improving the error handling with invalid inputs by user regarding the file extensions and plot engine - Improving the readability of the plotting scripts by merging the plot classes into Plot methods

Bachibouzouk · 2023-10-31T20:21:47Z

@mohammadamint - now it is fixed (I still have locally the old branch ramp_visualization in case)

Bachibouzouk

Thanks for the modifications @mohammadamint

Is the class Run being used at all? I couldn't find an instance of Run( in the whole project. I would say we leave it here and merge it into UseCase in a subsequent PR :)

mohammadamint · 2023-10-31T21:25:17Z

Thanks for the modifications @mohammadamint

Is the class Run being used at all? I couldn't find an instance of Run( in the whole project. I would say we leave it here and merge it into UseCase in a subsequent PR :)

Indeed I kept for the future UseCase uses.

Bachibouzouk · 2023-10-31T21:53:58Z

@mohammadamint - maybe we can merge the test PR first, then we rebase this one onto development and see that none of the tests is broken (which I would find surprising, but we never know). Then you could also edit the changelog to add a line about your contribution? What do you think?

mohammadamint · 2023-11-02T15:17:12Z

@mohammadamint - maybe we can merge the test PR first, then we rebase this one onto development and see that none of the tests is broken (which I would find surprising, but we never know). Then you could also edit the changelog to add a line about your contribution? What do you think?

Sure!

FLomb · 2023-11-06T10:14:16Z

The tests-related PRs have all been approved. You may proceed with rebasing and then we could merge this one too (:

mohammadamint · 2023-11-06T15:47:02Z

The tests-related PRs have all been approved. You may proceed with rebasing and then we could merge this one too (:

Shall I wait for your your PR on docs also to be merged?

FLomb · 2023-11-06T15:48:07Z

The tests-related PRs have all been approved. You may proceed with rebasing and then we could merge this one too (:

Shall I wait for your your PR on docs also to be merged?

yes, I think that would make sense. Is for one between you and @Bachibouzouk to review that one before merging.

FLomb · 2023-11-08T15:48:43Z

So I think that the only needed step here is not to update the PR to incorporate the newest commits to development, right?

mohammadamint · 2023-11-08T16:45:18Z

So I think that the only needed step here is not to update the PR to incorporate the n

Yes. I have some issues with rebasing, and if it is ok, I will wait till tomorrow to check this with Pierre. I see that some of the commits I had on this PR are kind of lost.

The new features added will allow the user to perform some data analysis and visualization routines: changes ------ *. ramp/post_process/post_process.py - the class Plot is the main class carrying on the plotting and data analysis functionalities with custom APIs providing access to some pd.DataFrame features also easy to plot functions conencting to plotly and matplotlibe methods. - different plotting classes that include a specific plot type visualiztion using two different engines (plotly & matplotlib) *. ramp.__init__.py - adding the Plot class to the init. documentation ------------- - the functions API docuemntation is added to the API documentaiton - A full example titled "Plot class" is added to the example gallery describing the full potential of the new functionalities

- Improving the Plot class docstring - Improving the error handling with invalid inputs by user regarding the file extensions and plot engine - Improving the readability of the plotting scripts by merging the plot classes into Plot methods

Bachibouzouk · 2023-11-08T21:06:16Z

I see that some of the commits I had on this PR are kind of lost.

Are there still lost @mohammadamint ? This rebase is onto the latest version developement branch

mohammadamint · 2023-11-08T21:13:17Z

I see that some of the commits I had on this PR are kind of lost.

Are there still lost @mohammadamint ? This rebase is onto the latest version developement branch

@Bachibouzouk All correct now!

Bachibouzouk · 2023-11-08T21:16:48Z

Ok now only the problem of RTD then. by looking at the dependencies locally, I see that docs/source/api_document is missing all the Plot methods, could this be that we just need to commit them?

mohammadamint · 2023-11-08T21:19:45Z

Ok now only the problem of RTD then. by looking at the dependencies locally, I see that docs/source/api_document is missing all the Plot methods, could this be that we just need to commit them?

@Bachibouzouk I see that the docs action is also failed. It can't find the API doc for Appliance.eq! Is that normal?

Bachibouzouk · 2023-11-08T21:26:38Z

@Bachibouzouk I see that the docs action is also failed. It can't find the API doc for Appliance.eq! Is that normal?

@mohammadamint The problem was the missing plotly dependency in the doc requirements, this is now fixed, the honor of merging is yours :) Congrats once more!

mohammadamint · 2023-11-08T21:28:19Z

@Bachibouzouk I see that the docs action is also failed. It can't find the API doc for Appliance.eq! Is that normal?

@mohammadamint The problem was the missing plotly dependency in the doc requirements, this is now fixed, the honor of merging is yours :) Congrats once more!

Thanks for your great support, and looking forward to your git course ;-)

mohammadamint requested review from Bachibouzouk and FLomb October 15, 2023 16:48

Bachibouzouk reviewed Oct 23, 2023

View reviewed changes

mohammadamint requested a review from Bachibouzouk October 24, 2023 16:39

Bachibouzouk reviewed Oct 27, 2023

View reviewed changes

Bachibouzouk linked an issue Oct 30, 2023 that may be closed by this pull request

Creating RAMP postprocessor #63

Closed

Bachibouzouk force-pushed the ramp_visualization branch 2 times, most recently from 3ad01e7 to 1c3d195 Compare October 31, 2023 16:47

Bachibouzouk force-pushed the ramp_visualization branch from 1c3d195 to 61af5b6 Compare October 31, 2023 20:19

Bachibouzouk self-requested a review October 31, 2023 21:09

Bachibouzouk approved these changes Oct 31, 2023

View reviewed changes

mohammadamint and others added 4 commits November 8, 2023 21:56

adding the new dependencies to the requirements

da26874

Update RTD

0c01599

PR #89 reviews

511d90b

- Improving the Plot class docstring - Improving the error handling with invalid inputs by user regarding the file extensions and plot engine - Improving the readability of the plotting scripts by merging the plot classes into Plot methods

Bachibouzouk force-pushed the ramp_visualization branch from 61af5b6 to 511d90b Compare November 8, 2023 21:05

Update docs requirements

5766f8c

mohammadamint merged commit a57b121 into development Nov 8, 2023
3 checks passed

Bachibouzouk deleted the ramp_visualization branch November 8, 2023 21:29

		elif file.split(".")[-1] == "xlsx":

		df = pd.read_excel(file,sheet_name=sheet_name,index_col=0,header=0)

Plot class for ramp visualization and data analysis routines (isse #63) #89

Plot class for ramp visualization and data analysis routines (isse #63) #89

Conversation

mohammadamint commented Oct 15, 2023

changes

documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bachibouzouk left a comment

Choose a reason for hiding this comment

mohammadamint commented Oct 23, 2023

Choose a reason for hiding this comment

Bachibouzouk commented Oct 27, 2023

mohammadamint commented Oct 30, 2023

Bachibouzouk commented Oct 31, 2023

Bachibouzouk commented Oct 31, 2023

mohammadamint commented Oct 31, 2023

Bachibouzouk commented Oct 31, 2023

mohammadamint commented Oct 31, 2023

mohammadamint commented Oct 31, 2023

Bachibouzouk commented Oct 31, 2023

Bachibouzouk commented Oct 31, 2023

Bachibouzouk left a comment

Choose a reason for hiding this comment

mohammadamint commented Oct 31, 2023

Bachibouzouk commented Oct 31, 2023

mohammadamint commented Nov 2, 2023

FLomb commented Nov 6, 2023

mohammadamint commented Nov 6, 2023

FLomb commented Nov 6, 2023

FLomb commented Nov 8, 2023

mohammadamint commented Nov 8, 2023

Bachibouzouk commented Nov 8, 2023

mohammadamint commented Nov 8, 2023

Bachibouzouk commented Nov 8, 2023

mohammadamint commented Nov 8, 2023

Bachibouzouk commented Nov 8, 2023

mohammadamint commented Nov 8, 2023