Skip to content

Conversation

@bayonato89
Copy link
Contributor

No description provided.

@wsavran
Copy link
Collaborator

wsavran commented May 4, 2021

this looks great @bayonato89 ! i haven't looked through the code in detail yet, but i did notice that i think we want to place these functions in csep/utils/plots.py. i will take a look through the code in the next couple of days.

@bayonato89
Copy link
Contributor Author

Removed the plot_diff_spatial_likelihood function from the poisson_evaluation.py file, and inserted it into the plots.py file.

@codecov-commenter
Copy link

Codecov Report

Merging #119 (f13a7d3) into master (40c28bb) will increase coverage by 0.58%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   56.88%   57.46%   +0.58%     
==========================================
  Files          19       19              
  Lines        3115     3120       +5     
  Branches      451      452       +1     
==========================================
+ Hits         1772     1793      +21     
+ Misses       1237     1217      -20     
- Partials      106      110       +4     
Impacted Files Coverage Δ
csep/core/poisson_evaluations.py 45.63% <ø> (ø)
csep/core/forecasts.py 55.93% <0.00%> (+0.84%) ⬆️
csep/core/catalogs.py 60.07% <0.00%> (+2.20%) ⬆️
csep/utils/calc.py 53.57% <0.00%> (+3.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40c28bb...f13a7d3. Read the comment docs.

@wsavran
Copy link
Collaborator

wsavran commented Jan 11, 2022

@bayonato89 i'd like to first add this function to the documentation to show an example of how to make a custom figure using matplotlib. having some kind of a cookbook could be cool and this figure might be a nice start. i can think of a few functions in the current plots.py module that could be better suited as cookbook recipes than included in the distribution. what do you think @bayonato89 @pabloitu @mherrmann3 ?

@mherrmann3
Copy link
Contributor

mherrmann3 commented Jan 12, 2022

Yes, documenting how to 'cook' custom plots is a good idea, but doesn't plots.py and @bayonato89's new plotting function already serve as guide by themselves? That is, CSEP users with Python experience will figure out how to "cook by themselves". The question is how often a regular CSEP user wants to create custom plots - and what type of plots?

For instance, this new plotting function of @bayonato89 shows a data layer on a map, which may be a common use case for which CSEP already has appropriate functions in place. But entirely new plots are rather difficult to anticipate. In case we want to document the 'cooking' in more detail, it may be sufficient to focus on one or more cases that adapt/extend one of CSEP's elemental plotting functions (e.g., a map) with a new feature (layer). For this purpose, I believe that @bayonato89's example is actually a good candidate to document. (side note: shouldn't this function make use of plot_basemap like plot_spatial_dataset does? In this case, plot_spatial_dataset is also a good candidate for illustrating how to adapt it to a custom layer.)

@wsavran: Did you intend to make this 'cookbook' a subsection of docs.cseptesting.org/tutorials/plot_customizations.html?

In addition, we might also include an enumeration of elemental CSEP plotting functions that have the potential to be extended/modified.

@wsavran
Copy link
Collaborator

wsavran commented Jan 12, 2022

@mherrmann3 I agree plots.py does that as well as the new function that @bayonato89 wrote. The main difference is where the information is found. In the case of plots.py users would have to read the code possibly from GitHub if they didn't install from source.

In the case of the 'cookbook' it would be a new section on the documentation page at the same level (or one below) tutorials. An example URL might read docs.cseptesting.org/cookbook/plot_log_likelihood_scores.html.

A main reason for this is that we could provide a place where users could submit 'recipes' (they don't necessarily have to be plotting functions) that aren't required to work with the pyCSEP plotting wrapper (ie plot_basemap and plot_spatial_dataset) or interface completely with the pyCSEP natives. The expectation would be that a user could copy-paste this function into their script and modify it as needed as opposed to us abstracting away every option that a new user might want (similar to the way plot_catalog or plot_spatial_dataset works). It seems like an efficient way to allow for useful new contributions while keeping the development requirements somewhat low.

Candidate functions to move into the cookbook would be

  • plot_cumulative_events_versus_time_dev
  • plot_cumulative_events_versus_time
  • plot_magnitude_versus_time
  • plot_ecdf
  • plot_magnitude_histogram_dev
  • plot_magnitude_histogram

@mherrmann3
Copy link
Contributor

Ah, now I got you. (I understood to create a place where some pyCSEP functions are described in more detail to help creating new ones).

So it's about a collection of optional functions. Yes, I like this idea! Basically like gist.github.com integrated into our docs. We could also think about shipping them with pyCSEP in a separate module (e.g., 'addons') to facilitate their use.

PS: should we move this discussion to a new issue?

@wsavran
Copy link
Collaborator

wsavran commented Jan 14, 2022

@mherrmann3 yes exactly! its kind of a place to allow for contributions that make sense to share with others but work standalone and dont' need to be integrated into the main package. i have started a notebooks folder that we might use for this. that way functions can be self documented within the package. im closing this pull request, but opening in an new issue to continue the discussion.

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