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

Add data plot to visualization #73

Merged
merged 7 commits into from
Apr 4, 2023
Merged

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Mar 17, 2023

Yes, maybe we can delete demo code after you approve and before merging

Feature or improvement description
data_plot was a function I'd found useful in the past for having an on hand way to represent noisy SCADA data (typical example might be achieved offset). It exists in a pretty out-dated version in FLORIS, so I've added this updated version to FLASC, and will submit another pull request to FLORIS to remove the outdate code

Impacted areas of the software
visualization.py

@paulf81
Copy link
Collaborator Author

paulf81 commented Mar 17, 2023

Noting related FLORIS pull: NREL/floris#606

Copy link
Collaborator

@Bartdoekemeijer Bartdoekemeijer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice functionality! See my comments. Cheers!

flasc/visualization.py Outdated Show resolved Hide resolved

# Small little demo of data plot, can delete
if __name__ == "__main__":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice example. It would be good to apply this plot somewhere in the flasc_cookiecutter_template repository on actual data so that users are made aware of its existence, perhaps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good idea, just gave myself a todo item to do that, maybe also we can use someplace in flasc's examples...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely helps for showing the usage of the function! In my opinion, this should probably be removed before the code is merged, since it is kind of a toy example. An option would be to add a more realistic usage somewhere in the examples, but perhaps just adding to the cookiecutter template at a later date is fine.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ Took too long writing my review, and Paul beat me to it :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, thinking that could be a seperate issue in the cookie_cutter repo? (To add an example?). I made a small one in the wake steering example but I think we have a broader discussion of where examples go so this could ultimately join that

flasc/visualization.py Outdated Show resolved Hide resolved
flasc/visualization.py Outdated Show resolved Hide resolved

# Small little demo of data plot, can delete
if __name__ == "__main__":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely helps for showing the usage of the function! In my opinion, this should probably be removed before the code is merged, since it is kind of a toy example. An option would be to add a more realistic usage somewhere in the examples, but perhaps just adding to the cookiecutter template at a later date is fine.


# If x_edges not provided, use 50 bins over range of x
if x_edges is None:
x_edges = np.linspace(df["x"].min()*.98, df["x"].max()*1.02, 50)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulf81 I changed the lower and upper edges from min*0.95, max*1.05 to min*0.98, max*1.02 to avoid numpy warnings that get caused because, combined the 50 bins, there will always be empty bins. An alternative, if you'd prefer, is to leave the edges at 0.95 and 1.05 but lower the bin count to 20, i.e.

x_edges = np.linspace(df["x"].min()*.95, df["x"].max()*1.05, 20)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do see that the warnings that I was trying to avoid are still being raised in the example, I think because the data isn't really spread along the x axis in a realistic way. For now, I don't think that's really a problem.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are using the example dataset, absolutely the data is mostly in really low wind speeds, indeed unrealistic. This has been an annoyance for me while working with this data, for sure. I wasn't sure if we wanted to fix this or just wait for the revision of the examples to finish.

Comment on lines +724 to +729
df_agg["y_ci_lower"], df_agg["y_ci_upper"] = st.t.interval(
confidence_level,
df_agg["y_count"]-1,
loc=df_agg["y_mean"],
scale=df_agg["y_sem"]
)
Copy link
Collaborator

@misi9170 misi9170 Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulf81 I think what you have, using the t-distribution, is good (it's a little more correct than the z-score approach, which assumes the data set is large enough that the means can be treated as normal---the t-distribution doesn't make that assumption, and approaches the z-score method as the number of samples becomes large). I've rearranged the code a little in my commits, but have not changed the method.

@paulf81
Copy link
Collaborator Author

paulf81 commented Mar 30, 2023

Hi @Bartdoekemeijer and @misi9170 what do you think, good to merge this one? I know we have some open conversations but unless I misread we can put off full solutions to these to other activities?

@misi9170
Copy link
Collaborator

misi9170 commented Apr 4, 2023

Hi @Bartdoekemeijer and @misi9170 what do you think, good to merge this one? I know we have some open conversations but unless I misread we can put off full solutions to these to other activities?

@paulf81, good by me.

@paulf81
Copy link
Collaborator Author

paulf81 commented Apr 4, 2023

Squashing and a'merging

@paulf81 paulf81 merged commit 987cd82 into NREL:develop Apr 4, 2023
@paulf81 paulf81 deleted the feature/data_plot branch April 4, 2023 21:32
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.

3 participants