First pass overall API structure

[emiliom]

(Previously I started this issue with a draft recommendation and comments. After our meeting, I've re written this to reflect our decisions)

API re-organization (subpackages > modules) plan

Sub-packages and modules under them

• loadinputs (or input)
  • load_biological_data.py
  • load_kriging_data.py
    • New module, with code moved from kriging_mesh.py). Note that I'm proposing the name load_kriging_data.py rather than load_kriging_mesh.py because it loads both the mesh and the smoothed contour data
  • load_nasc_data.py
  • load_stratification_data.py
• computation (or compute verb, for consistency with other subpackage names)
  • compute_biomass_density.py
  • cv_analysis.py
  • kriging_mesh.py
    • Move to new loadinputs/load_kriging_data.py module the elements that load the mesh and smoother contour data
  • kriging.py
  • run_bootstrapping.py
  • semivariogram.py
• export
  • generate_reports.py
  • visualization.py
• utils
  • input_checks.py

"Root" level, https://github.com/uw-echospace/EchoPro/tree/master/EchoPro/

• __init__.py
  • Note: remove from __future__ import from it
• survey.py
  • Move from survey/survey.py

Other, related changes

• create_preliminary_files/create_files.py: Remove this module and its parent subpackage. It's currently just a small stub and it's unclear yet to what extent we'll build this "pre-processing" functionality.
• global_vars.py: It encompasses only kriging and semivariogram variables. Either move it to the new computation subpackage, or, preferably if not too difficult at this stage, eliminate the use of global-scope variables altogether.
• numba_modules.py: Break apart and move individual functions to the corresponding subpackages or to the modules where they're used, if not used in multiple modules.

[leewujung]

To recap from yesterday since there is no notes here:
My recommendation is to lump what's under kriging and other, core into a single subpackage called computation. We can discuss the subpackage name again at a later stage, but having the grouping done will allow us to proceed with additional develoments.

Also, I spaced out when we were discussing preprocessing and do not have any re-collection on the discussions... Can this go into loadinputs?

@emiliom : could you modify the issue text above or add a comment to capture the results from yesterday? Thanks!

[emiliom]

@leewujung I've replaced my original text above with the plan we came up with (plus some additional suggestions or questions). That seemed cleaner than trying to preserve the original text.

[leewujung]

@emiliom : thanks -- looks great!
How about changing loadinputs to dataloader so that everything is a noun at the subpackage level?

Along the same line, I think it'll be nice to have all the modules (each scripts within the subpackages) to follow the verb or verb_noun pattern, for consistency.

[emiliom]

How about changing loadinputs to dataloader so that everything is a noun at the subpackage level?

That would work.

Along the same line, I think it'll be nice to have all the modules (each scripts within the subpackages) to follow the verb or verb_noun pattern, for consistency.

Sure. Can you suggest the new module names, for those that would need to be changed?

[leewujung]

After looking through it I decided to go with all nouns, since the current ones are mostly nouns.

Note: I removed the computation/kriging_mesh.py entry to avoid confusion, since it will be moved to under data_loader.

Sub-packages and modules under them

• loadinputs (or input) --> data_loader
  • load_biological_data.py --> biological_data.py
  • load_kriging_data.py --> kriging_data.py
    • New module, with code moved from the original kriging_mesh.py. This module loads both the mesh and the smoothed contour data
  • load_nasc_data.py --> nasc_data.py
  • load_stratification_data.py --> stratification_data.py
• computation
  • compute_biomass_density.py --> biomass_density.py
  • cv_analysis.py --> cv.py
  • kriging.py (no change)
  • run_bootstrapping.py --> bootstrapping.py
  • semivariogram.py (no change)
• export (no change)
  • generate_reports.py --> reports.py
  • visualization.py (no change)
• utils --> no change
  • input_checks.py (no change)

[b-reyes]

@leewujung and @emiliom thank you for outlining and discussing the rearrangement of the API structure. @leewujung I think all of the names you have provided look good.

There is one small item we need to discuss. I do agree with moving the data loading portion of kriging_mesh.py to kriging_data.py. However, kriging_mesh.py contains more than just loading the mesh and smoothed contour data. It also contains routines that transform, reduce, and plot the Kriging mesh and smoothed contour data. What would we like to do with these routines? Do we want to move them into kriging.py or do we want to create computation/kriging_mesh.py that will hold these routines?

[leewujung]

@b-reyes : good question. Thanks for bringing it up since I did not look into what the module actually contains. I just took a quick look at the current kriging_mesh/kriging_mesh.py.
What do you think about the following split?

• In data_loader/kriging_mesh.py: routines that transform, reduce the Kriging mesh.
  My thought is that it is common for data loader to contain components that crop and transform the data in ML setups, so this seems reasonable.
• Separate out the plotting functions get_folium_map, plot_points, plot_layered_points
  Some changes would be needed to make in particular plot_layered_points to accept a KrigingMesh object.
  Seems a good idea to have those near the other viz functionalities so that we have all things related to plotting in one place.

So now I don't like the export subpackage name because export and visualize to me are two different things.
But, to get us going, maybe we just run with it for now.
How about the following?

• export (no change)
  • generate_reports.py --> reports.py
  • visualization.py: for now this can hold both the mesh viz functions and other viz functions to be created, and we can decide if we want to split them down further when we work on the function design.

[b-reyes]

In data_loader/kriging_mesh.py: routines that transform, reduce the Kriging mesh.
My thought is that it is common for data loader to contain components that crop and transform the data in ML setups, so this seems reasonable.

@leewujung this seems reasonable, it would basically amount to just moving the current kriging_mesh.py module to data_loader minus the visaulization portions.

How about the following?

• export (no change)
• generate_reports.py --> reports.py
• visualization.py: for now this can hold both the mesh viz functions and other viz functions to be created, and we can decide if we want to split them down further when we work on the function design.

From what I understand you would like to move all visualization type functions to the module visualization.py. Would this change be reflected in how the user utilizes these routines? For example, right now we plot Kriging results using the following krig.plot_kriging_results(...), where krig is a Kriging object. Would your changes want to keep this or do something like EchoPro.visualization.plot_kriging_result(krig)?

[leewujung]

I like EchoPro.visualization.plot_kriging_result(krig)!

Or maybe a step further (for brainstorming later):
EchoPro.plot(krig) -- the plot function is "smart" to look at the input object and produce some canned plots. We just need a layer between the top level API and the modules under visualization.

[b-reyes]

I like EchoPro.visualization.plot_kriging_result(krig)!

Great, I will go ahead and implement this along with the mesh plotting functions.

Or maybe a step further (for brainstorming later):
EchoPro.plot(krig) -- the plot function is "smart" to look at the input object and produce some canned plots. We just need a layer between the top level API and the modules under visualization.

Yes, I had a similar thought because this would look very nice. However, I feel like this might not be ideal because the plotting functions plot_kriging_results, plot_layered_points, plot_points all have different types of input. I think this would make creating a general EchoPro.plot function difficult and possibly unintuitive for the users. We can discuss this further in another issue, if we think it is work pursuing.