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

Preprocessing changes #155

Merged
merged 17 commits into from
Feb 28, 2023
Merged

Preprocessing changes #155

merged 17 commits into from
Feb 28, 2023

Conversation

evcano
Copy link
Member

@evcano evcano commented Feb 17, 2023

Hi @bch0w,

As I mentioned in issue #152, I made the following changes to the preprocessing module:

  1. Synthetics and observations can have a different format. This change was on the default preprocessing module. However, I changed the name of the variable <data_format> in the solver and pyaflowa modules in order to keep consistency. I also updated the tests, examples, and documentation to account for my changes.
  2. Observations can have SAC format
  3. Added <unit_output> variable to default preprocessing module
  4. Added hard check: the observed and synthetic data format must be correct
  5. Added hard check: synthetic and observations files must match
  6. Added function to check that all required adjoint sources exist
  7. Fixed: Adjoint traces were incorrectly written. Adjoint traces were initialized as a copy of the obspy Stream containing the synthetic trace. Then, the adjoint trace was written in a new property of the Stream called "data" instead of being written on the data property of the Trace object contained in the Stream. This resulted in the synthetic trace being written instead of the adjoint trace.

Feel free to modify what you consider necessary.

Best,
Eduardo.

@bch0w
Copy link
Member

bch0w commented Feb 20, 2023

Hi @evcano, thanks for the PR. Just FYI I'm out of town the next few days but will check this out later this week!

@evcano
Copy link
Member Author

evcano commented Feb 28, 2023

Hi @bch0w, I think I fixed an important bug in the default preprocessing module. Please see point 7 of my PR.

@bch0w
Copy link
Member

bch0w commented Feb 28, 2023

Hi @evcano, wow! Great catch. That's a pretty sneaky bug, I'm surprised it worked and provided usable results in the examples. Thanks very much for going through the preprocess module with such detail, it's incredibly useful!

Sorry it's taken me so long to get to this PR, I got caught up in some other work. I've had a look through all the proposed changes and everything looks good to me. Just want to run a few example problems on my local to test it out and then I will merge the PR.

@evcano
Copy link
Member Author

evcano commented Feb 28, 2023

Hi @bch0w, no problem! Take your time

Copy link
Member

@bch0w bch0w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks great @evcano! Just one suggestion about amending a docstring to avoid repeated parameters in the dynamically generated parameter file. Once that is done, I am happy to merge this into the devel branch. Thanks!

@bch0w bch0w merged commit f5a3810 into adjtomo:devel Feb 28, 2023
@bch0w
Copy link
Member

bch0w commented Feb 28, 2023

Hi @evcano, I just decided to push the suggestion myself since it was small. Hope that's alright. Thanks again for the contribution!

@evcano
Copy link
Member Author

evcano commented Mar 1, 2023

Of course, thanks for modifying it for me. I am glad to help!

@evcano evcano deleted the preprocessing_changes branch March 7, 2023 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants