sourcespec icon indicating copy to clipboard operation
sourcespec copied to clipboard

v2_ssp_func

Open krisvanneste opened this issue 1 year ago • 1 comments

Claudio,

Here is my first attempt to reorganize sourcespec in a way that it can be run as a function. I don't think I wrote more than 5 new lines, but most of the changes consisted of moving existing code to new functions:

  • in ssp_setup.py I added a new get_outdir_path function
  • in ssp_read_traces.py I added new read_station_inventory, read_event_and_picks, augment_event, augment_traces and select_components functions
  • in source_spec.py, I added new ssp_run and ssp_output functions

I haven't been able to test it yet. I will need to modify my own code quite substantially before that will be possible, but I will first try writing a simple notebook to see if I can make it work.

You will probably want to organize the new functions in different files. Also,don't hesitate if you want to change the names of these new functions.

In some places, I added some TODO lines regarding alternative decisions that could be taken or questions that I have.

If you have time, please have a look and let me know if this makes sense.

Kris

krisvanneste avatar Jul 11 '24 09:07 krisvanneste

Thanks Kris!

Only five additional lines is perfect 😆 . I think we should keep these pull requests sufficiently small to be able to be easily reviewed: more pull requests will follow 😉

Not sure I'll have time this week to review the PR, but I'll certainly work on it next week!

claudiodsf avatar Jul 11 '24 09:07 claudiodsf

Hi Kris,

I made some modifications:

  • Improved docstrings
  • Necessary code modifications to run in CLI

The main problem was that config.event is not available before calling augment_event(), so I choose to explicitly pass the eventid to many functions.

I think that we should reflect on whether we want event to be within config: this is the only input data that is treated this way (traces, metadata, spectra are all explicitly passed to functions).

Anyway, we can keep this change for later, since it is quite a major one.

Other point: it is not clear to me why read_station_inventory() is needed, since it just calls read_station_metadata().

Finally, to test the code, you have to force-pull, since I rebased. Note that all the squashme- commits are supposed to be squashed within the previous one (I will do it just before merging this PR).

claudiodsf avatar Jul 15 '24 11:07 claudiodsf

Claudio, I force-pulled the branch and my notebook still works!

  • I missed the potential problem with config.event when running the CLI, but indeed we should investigate if this can be handled differently in the relevant functions.
  • There is indeed no need for the read_station_inventory function.
  • In the mean time, I added a cell to the notebook to run the ssp_output function using a temporary directory with all save options deactivated and config.plot_show = True. I obtain plots of the traces, the fitted spectra, the spectral weights, the stacked spectra and the boxplot. Only 3 files are written: <event_id>.residuals.hdf5, <event_id>.ssp.out and <event_id>.ssp.yaml. Would you consider configuration options for each type of plot and output?
  • I tried changing the logging level, but it did not work. We will probably have to work on the logging setup, to make it work irrespective of whether it is written to a file or not.

krisvanneste avatar Jul 15 '24 12:07 krisvanneste

Thanks for the feeback.

I'm going to merge this one and start working on reorganize the setup and read input part of the code into subdirectories.

I'll also take care of the two Config().__init__() fixes mentioned here.

It would be nice if you could send me a working script or notebook to test the API all along the modifications 😉

claudiodsf avatar Jul 15 '24 14:07 claudiodsf

Claudio, thanks. To avoid my specific functions to read event data, phase picks and waveform data, I can write everything into a ASDF file, which is easier to share. I need to work out what would be the easiest way to read all pieces of information from the ASDF file, and maybe add support for that in sourcespec. What would be the easiest way to share the notebook and data file(s)?

krisvanneste avatar Jul 15 '24 14:07 krisvanneste

What would be the easiest way to share the notebook and data file(s)?

For the moment, just send me an email or a transfer link? We will see later what to include as an offical example 😉

claudiodsf avatar Jul 15 '24 14:07 claudiodsf

Kris, please take a look at these two commits:

  • https://github.com/SeismicSource/sourcespec/commit/5b5a0fa200dcbcd63e2ac0b1944908f91ff49743 : in-place select_components() (I wonder if select_components() could be merged into augment_traces())
  • https://github.com/SeismicSource/sourcespec/commit/233222f967523dece7a766a502b2f52988c24119 : config submodule renamed to setup and content of ssp_setupy.py split in three files

This will require some adjustment on your side 😉

claudiodsf avatar Jul 15 '24 16:07 claudiodsf

I further moved all the input functions into an input subdirectory (submodule).

https://github.com/SeismicSource/sourcespec/commit/098f3f2cd83e3beec73c2dd427033946b7dee3ff

Also, I moved select_components() into augment_traces()

claudiodsf avatar Jul 16 '24 10:07 claudiodsf

You are doing quite a bit of refactoring, but it looks good! I only had to modify one import line in the notebook.

krisvanneste avatar Jul 16 '24 12:07 krisvanneste