Skip to content

Remove plotting module in favor of NREL/flasc#606

Merged
paulf81 merged 1 commit intoNatLabRockies:developfrom
paulf81:feature/delete_plotting
Mar 23, 2023
Merged

Remove plotting module in favor of NREL/flasc#606
paulf81 merged 1 commit intoNatLabRockies:developfrom
paulf81:feature/delete_plotting

Conversation

@paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Mar 17, 2023

Deleting Plotting.py

Plotting.py contains functions that we used to use in connection with FLORIS, but this type of thing now belongs better in flasc, and much of the code hasn't been updated in a while. Propose a little "garbage collection" and we delete this file rather than leave the bloat.

FYI: Related pull request in FLASC is here: NatLabRockies/flasc#73

Impacted areas of the software

plotting.py

@rafmudaf rafmudaf changed the title Delete plotting.py Remove plotting module in favor of NREL/flasc Mar 17, 2023
@rafmudaf
Copy link
Collaborator

This looks good to me. Is there anything else in FLORIS that could be removed in favor of flasc?

@misi9170
Copy link
Collaborator

misi9170 commented Mar 17, 2023

This looks good to me. Is there anything else in FLORIS that could be removed in favor of flasc?

The other things that come to me are also plotting/visualization functions, such as visualize_layout() from layout_functions.py. That functionality is now further built out in FLASC. I could see this both ways:

  • On one hand, visualize_layout() could get stale because we (the developers) will likely always use FLASC's tools for this
  • On the other hand, if there are some users using this functionality and none of FLASC's other functionality, it might be a bit of a disservice to cut it.

@paulf81 @rafmudaf Thoughts?

@rafmudaf
Copy link
Collaborator

That's a good point, @misi9170, and it probably deserves a larger discussion on the long term roadmap for FLORIS, FLASC, and any other tools that integrate into the typical workflows centered on FLORIS.

What is the current state of FLASC - fairly stable or more active development planned? If it's at a point where it will not fundamentally change much going forward, I think it would be good to consolidate some of these functionalities. If more changes are in the works, though, it might be best to keep this in FLORIS while we plan out a future direction.

The things happening in FLASC go well with the efforts we've been thinking about to design a better, more cohesive floris.tools API. With that in mind, I'm maybe more in favor of leaving things as they are in FLORIS and expecting that in a future version they'll be completely removed in favor of FLASC.

@Bartdoekemeijer
Copy link
Collaborator

What is the current state of FLASC - fairly stable or more active development planned? If it's at a point where it will not fundamentally change much going forward, I think it would be good to consolidate some of these functionalities. If more changes are in the works, though, it might be best to keep this in FLORIS while we plan out a future direction.

This is really in the hands of @misi9170 and @paulf81, but I'll share my two cents as an industry user and supporting developer of the code. I think the underlying approach in FLASC is solid: how we deal with data, the format and style conventions we use, the ways classes interface with one another, and how we calculate the energy ratios. In that sense, it's a fully usable and solid tool. I know of at least one other developer and one research institute using the tool. On the other hand, there is still substantial development ongoing to expand functionality and tweak classes, and the API still changes a little bit too often in my eyes. I think a couple of things to prioritize going forward:

With this in place, any user will be able to easily create a tailored repository for their wind farm application, e.g., SCADA analysis, model validation, wake steering assessment, and layout optimization. All the user has to do in take the Cookiecutter package, go through the Cookiecutter CLI, and replace the example files with the site-specific information. The user will not have to worry about downloading the right versions of FLORIS or FLASC -- everything just comes through pip. The additional benefit is that repositories will still work out of the box years after their development, since we can set their requirements.txt to the then current FLASC version.

The things happening in FLASC go well with the efforts we've been thinking about to design a better, more cohesive floris.tools API. With that in mind, I'm maybe more in favor of leaving things as they are in FLORIS and expecting that in a future version they'll be completely removed in favor of FLASC.

I think it makes sense to keep some basic functionality in FLORIS, like layout plots and the /examples/ folder. The Cookiecutter template is the right choice for the power users, but people learning Python and exploring FLORIS may find the FLORIS repository an easier entry.

@misi9170
Copy link
Collaborator

misi9170 commented Mar 21, 2023

@rafmudaf @Bartdoekemeijer

I totally agree with everything that both of you have said. While FLASC is, I would say, stable, it is still under active development with a lot of new functionality being added around the core code. Furthermore, I do believe that some (likely many) users will want basic layout plotting as provided by FLORIS already and may not need the more extensive layout plotting tools that FLASC has, and I think it's best we don't ask these users to clone and install FLASC just for this. As @Bartdoekemeijer points out, this could be a different story when FLASC is on PyPI and easily pip-installable.

So, to sum up, I support leaving visualize_layout() in FLORIS for the time being, and revisiting at a later date when FLASC is more mature. The removal of plotting.py though (as originally suggested by @paulf81 in this pull request) I think is fine, since this is functionality that I believe is strongly linked to processing SCADA data, which is FLASC's domain now.

@paulf81
Copy link
Collaborator Author

paulf81 commented Mar 22, 2023

Thanks @rafmudaf @Bartdoekemeijer and @misi9170 for the discussion!! I think we're good now to merge this pull request, and then we can keep discussing other features in discussion threads. @rafmudaf ok by you for me to merge this one in?

@rafmudaf
Copy link
Collaborator

@paulf81 That's fine by me. @bayc is still listed as a reviewer, is that still needed?
When it is merged, please use "Squash and merge".

@paulf81 paulf81 merged commit 7a9dba7 into NatLabRockies:develop Mar 23, 2023
@paulf81 paulf81 deleted the feature/delete_plotting branch March 23, 2023 04:04
@paulf81
Copy link
Collaborator Author

paulf81 commented Mar 23, 2023

Squashed and merged! Thanks!

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.

4 participants