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

v2_ssp_func #54

Merged
merged 4 commits into from
Jul 15, 2024
Merged

v2_ssp_func #54

merged 4 commits into from
Jul 15, 2024

Conversation

krisvanneste
Copy link
Collaborator

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

@claudiodsf
Copy link
Member

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
Copy link
Member

claudiodsf commented Jul 15, 2024

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).

@krisvanneste
Copy link
Collaborator Author

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.

@claudiodsf
Copy link
Member

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 😉

krisvanneste and others added 3 commits July 15, 2024 16:05
…new select_components and augment_traces functions.

Moved code from read_traces function to new read_station_inventory, read_event_and_picks and augment_event functions.
Since the `event` object might not be yet in the `config` object.
@claudiodsf claudiodsf marked this pull request as ready for review July 15, 2024 14:07
@claudiodsf claudiodsf merged commit a3dabed into SeismicSource:v2 Jul 15, 2024
2 checks passed
@krisvanneste
Copy link
Collaborator Author

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)?

@claudiodsf
Copy link
Member

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
Copy link
Member

Kris, please take a look at these two commits:

  • 5b5a0fa : in-place select_components() (I wonder if select_components() could be merged into augment_traces())
  • 233222f : config submodule renamed to setup and content of ssp_setupy.py split in three files

This will require some adjustment on your side 😉

@claudiodsf
Copy link
Member

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

098f3f2

Also, I moved select_components() into augment_traces()

@krisvanneste
Copy link
Collaborator Author

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

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.

None yet

2 participants