search

geodesymiami / Precip

GNU General Public License v3.0
4 stars 0 forks source link

Kawan's comments #4

Open falkamelung opened 1 month ago

falkamelung commented 1 month ago

I’m unfortunately still having some issues with plot_precip.. The errors that I am having now aren’t raising an error but instead the process is being automatically killed due to out of memory issues, this happened quite a few times with the Cotopaxi and I think other volcanoes. I’m not sure if this is unique to jetstream2. There is also still the issue with the name/latlon switch for some map plots.

@giacomo if you end up having time next week to start the refactor, I would suggest that you open a new branch, called refactor and start working on the changes there.

A couple things that we should prioritize for the refactor include:

  1. Break up prompt subplots and simplify. (see the last commit for my suggestions)
  2. A plot function should take the axes as an input parameter. This would allow greater control of the plot and means we can make one big figure of n subplots and pass each axes to a plotting function. Currently plt... is being used. This is a bigger change and you will need to get familiar with matplotlib to do so. Long term, this change will be really worth it. https://matplotlib.org/stable/users/explain/axes/index.html https://stackoverflow.com/questions/43482191/matplotlib-axes-plot-vs-pyplot-plot
  3. Less inps arguments (start with positional/volcanoname and all the download options) - at the moment I need to comment out the 'inps.volcano_name == inps.positional' line if I want to use run_all..
  4. Read through minty cli sccripts and plot functions for good templates.

Initial comments:

//precipvm/home/exouser/code/rsmas_insar/tools/Precip[2021] grep -r . -e 'KA:' ./src/precip/plotter_functions.py:# KA: Don't use import * ./src/precip/plotter_functions.py:# KA: This function should just handle plotting. ./src/precip/plotter_functions.py:# KA: Ideally it takes data and a single axes as the input and then sends this data to another function depending on style ./src/precip/plotter_functions.py: # KA: this is a useful function but should be moved to the command line script ./src/precip/plotter_functions.py: # KA: this is a useful function but should be moved to the command line script ./src/precip/plotter_functions.py: # KA: There is no else statement here and isn't there always a style if you want to plot something? ./src/precip/plotter_functions.py: # KA: lets keep saving outside of plotting functions for now ./src/precip/plotter_functions.py: # KA: no need to set an extra bool ./src/precip/plotter_functions.py: # KA: instead -> if inps.style in ['daily', 'bar', 'strength': ./src/precip/plotter_functions.py: # KA: This dataframe should be the input parameter to a plotting function so that it doesn't have to be read everytime ./src/precip/plotter_functions.py: # KA: This dataframe should be the input parameter to a plotting function so that it doesn't have to be read everytime ./src/precip/plotter_functions.py: # KA: This dataframe should be the input parameter to a plotting function so that it doesn't have to be read everytime

falkamelung commented 1 month ago

For each volcano/run it should generate one or more files that people can download if they want to.

When everything is nicely running we need to support additional datasets: