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

Begin repo reorganization for v2 #165

Merged
merged 25 commits into from
Feb 7, 2024
Merged

Conversation

misi9170
Copy link
Collaborator

@misi9170 misi9170 commented Feb 2, 2024

One of the main changes we'd like to implement for FLASC v2 is a reorganization and consolidation of the repository. This includes:

  1. Reorganizing the repository into more cohesive sets of tools, and renaming the modules to reflect their key functionality
  2. Consolidating similar tools, and grouping similar tools between files
  3. Removing redundant code or code that no longer makes sense to keep in FLASC

This PR begins to address 1. and makes a few steps towards 3. In particular:

  • raw_data_processing, most of turbine_analysis, and dataframe_operations are grouped into a single module preprocessing (however, I'm not entirely sure about the name "preprocessing"---I'm interested in hearing opinions on this)
  • model_tuning, model_estimation, and some of turbine_analysis are consolidated into a single module model_fitting
  • the energy_ratio module is renamed analysis for generality
  • various utilities are grouped into a top-level module
  • wake_steering is dissolved (some going to utilities, some going to the top level for lack of a better option at this stage)

Some questions remain:

  • There are module-specific utilities that have stayed within the various modules but that could instead be moved to the utilities/ module (in particular, model_fitting/tuner_utils.py and analysis/energy_ratio_utilities.py)
  • The remaining top-level packages (optimization.py, visualization.py, and yaw_optimizer_visualization.py) should perhaps not be top-level packages; I just wasn't sure where to put them. However, some visualization capabilities will likely be removed and moved to Floris (see here)
  • The names of the files within each module have been left for the time being, but some will likely be updated in future pull requests as we streamline the repository

Notes

Tests currently fail because the CI workflow is pointing at the to-be-released Floris v4. This is intentional, and various pieces of FLASC will need to be updated to get these tests passing prior to the release of FLASC v2. However, all tests pass when Floris v3 is used instead.

Partly addresses #144 but does not close it.

@misi9170
Copy link
Collaborator Author

misi9170 commented Feb 2, 2024

@paulf81 @Bartdoekemeijer @ejsimley It would be great to get your input on the updated structure; happy to iterate on this a bit before merging.

@Bartdoekemeijer
Copy link
Collaborator

I like the new folder structure! Makes a lot of sense to me.

@paulf81
Copy link
Collaborator

paulf81 commented Feb 5, 2024

hi @misi9170 this is looking really good! I have a few suggestions, either for this pull request, or to be flagged for the next step. But I should say I would say go ahead and make this the base v2 branch whenever you like. Here are my thoughts:

  • Maybe data_processing is a better term than preprocessing (I know I proposed preprossing but agree with you it is not accurate for all codes)
  • Or maybe data_filtering_and_processing?
  • Somehow I think the files northing_offset.py (currently in preprocessing) and energy_ratio_wd_bias_estimation.py (analysis) should be grouped together. In the past they were seperated because one was based on pure "data" and the other on energy ratios, but functionally they are pursuing similar ends and I think clearer if they are together.
  • @Bartdoekemeijer ok if we delete the file df_reader_writer.py? We don't see any references in the code to these functions
  • Just a reminder to myself to update requirements such that FLORIS>4 required, and we remove things we aren't using sqlalchemy

@Bartdoekemeijer
Copy link
Collaborator

@Bartdoekemeijer ok if we delete the file df_reader_writer.py? We don't see any references in the code to these functions

Yes good to delete. This way mainly there to accommodate very large datasets, e.g., 1-second data of a couple months, that wouldn't fit in a single data file. I haven't used it and probably would require some rework anyway. Good to delete -- can always reintroduce if necessary.

@ejsimley
Copy link
Collaborator

ejsimley commented Feb 7, 2024

I think the new structure looks good overall. I just had a few thoughts on the organization:

  • I'm wondering if we need a separate "preprocessing"/"data_processing" directory, or if those modules can be included in utilities. Some of the modules like "time_operations" seems like a natural fit for utilities. In utilities, there could be submodules to keep the folder organized, like "filtering" or "dataframe_operations" that contain a few files each (For example, "filtering" could contain "find_sensor_faults" and "ws_pow_filtering"). Though too many submodules can make importing statements long I suppose.
  • I agree, some of the module-specific utilities could be moved to utilities. "tuner_utils" seems like a more general module that could be moved. "Energy_ratio_utils" might be more specific to the energy ratio or "analysis" module, though.
  • As far as top-level modules, maybe "optimization" could get moved to utilities.

@misi9170
Copy link
Collaborator Author

misi9170 commented Feb 7, 2024

@ejsimley Thanks for the suggestions! I went ahead and moved tuner_utils.py, energy_ratio_utilities.py, and optimization.py to utilities/ (and renamed "tuner_utils" to "tuner_utilities" for consistency).

I decided to leave the data_processing module alone for now. I feel that the intention of this code is still distinct from utilities, which I see as a set of helper functions. However, there may be pieces of data_processing that should be moved to utilities; I'm imagining that there will be a second round of consolidation/reorganizing before v2 is released, and I'll keep that in mind.

@paulf81 I've also taken your suggestions into account. I'm going to add the sqlalchemy point to issue #168

@misi9170 misi9170 requested a review from paulf81 February 7, 2024 18:49
Copy link
Collaborator

@paulf81 paulf81 left a comment

Choose a reason for hiding this comment

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

I approve. I think we should set this now as the v2 branch and then issue/pr further changes one-by-one to it, thank you @misi9170 !

@misi9170 misi9170 merged commit 9c7b004 into NREL:v2 Feb 7, 2024
0 of 3 checks passed
@misi9170 misi9170 mentioned this pull request Feb 7, 2024
3 tasks
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