adjtomo / seisflows

An automated workflow tool for full waveform inversion and adjoint tomography
http://seisflows.readthedocs.org
BSD 2-Clause "Simplified" License
172 stars 122 forks source link

Are people actively using preprocess.default.py? There appears to be several bugs #173

Closed Ben-J-Eppinger closed 3 months ago

Ben-J-Eppinger commented 1 year ago

I am trying to switch form my stable fork of seisflows to seisflows 3. I work at exploration scales so usually just need to use the options provided in the default pre-processing module. However, I feel like there are several bugs in this implementation. For example, the muting functions don't appear to be implemented correctly because the source and receiver coordinates appear set to the same variable (the src_cords = get_reciever_cods(st)). That is just one example, I have found and fixed several other bugs and continue to find more. I am asking because I am not sure if I am making silly mistakes, or trying to use functionally that is ostensibly extinct/unused

bch0w commented 1 year ago

Hi @Ben-J-Eppinger, thanks for raising this issue.

You are definitely not making silly mistakes, and apologies if you've run into any issues using the default preprocessing module. I do use the default preprocessing module, but only the filtering portion. Many of the functions like muting and tapering that are useful at the exploration scale were rewritten from the original code, but have not been tested or used extensively, which is why there are likely several bugs floating around.

If you would like to open up a pull request to submit bugfixes that you find, we would really appreciate it! Alternatively, if you point out any bugs you find here in this issue, I'd be happy to go in and fix those as soon as possible to get this module running smoothly!

Ben-J-Eppinger commented 1 year ago

Hi Bryant, thanks for getting back to me so quickly! I would be happy to submit some bug fixes to the repository, but upon further inspection, more of my problems seem to be coming from the fact that I am trying to work in the SU format. For example, the _check_adjoint_traces function works for the ASCII files specfem generates but not the SU ones. More importantly, the method of writing separate adjoint source files for each station rather than having one for each event seems to not be what specfem is expecting when using the SU format. I have been able to work past several of these issues, however, I am sure I have managed to do so in a way that doesn't break other functionality of the code, so I am not quite sure how best to proceed. I work in the near-surface, so I wonder if my use case is pretty outlandish compared to what most people are doing with Seisflows these days.

bch0w commented 1 year ago

Hi @Ben-J-Eppinger, just wanted to let you know I've seen this message and your post on the issue regarding the multi-core workflow, but I'll be out in the field the next few days so won't be able to get back to you until next week. I'll try to respond to you asap!

Ben-J-Eppinger commented 1 year ago

No worries, I really appreciate the help! This issue really has me stumped so I am trying/looking forward to learning more.

On Wed, Jul 5, 2023 at 6:18 PM Bryant Chow @.***> wrote:

Hi @Ben-J-Eppinger https://github.com/Ben-J-Eppinger, just wanted to let you know I've seen this message and your post on the issue regarding the multi-core workflow, but I'll be out in the field the next few days so won't be able to get back to you until next week. I'll try to respond to you asap!

— Reply to this email directly, view it on GitHub https://github.com/adjtomo/seisflows/issues/173#issuecomment-1622085590, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVKUFM3UVKSPALROEDGKFW3XOWHWRANCNFSM6AAAAAAZ4WOCBI . You are receiving this because you were mentioned.Message ID: @.***>

bch0w commented 11 months ago

Hi @Ben-J-Eppinger, back in the office and working on SeisFlows again.

I would say your use case is exactly what SeisFlows should be used for! And it's partly on me for dropping the ball on carrying over support for things like SU format, which were available in the original codebase.

It would be great to work together on this as I am not very familiar with common preprocessing techniques required at exploration scales. I'd be excited to ensure that you can use the latest codebase for your own work. Perhaps a good plan of action is to for me to get SU format support working, and then we can identify functionality that still requires some work, such as muting?

If that sounds good to you, I may rename this thread and we can continue discussing the SU and preprocessing fixes here.

Ben-J-Eppinger commented 11 months ago

I agree that it would be great to work together on this! It is our field season, so I apologize in advance if my responses are slow.

I am not entirely sure what the best way to proceed is, but let me explain my current understanding of the issue and I would love to get your thoughts on how to fix it. There is a major difference between specfem2d and specfem3d in that regardless of the seismogram output format selected in the Par_filel (ASCII, SU, etc), specfem3d will always write a file for each trace. In specfem2d however, the number of output files written depends on the type of out format selected. For example, when the ASCII format is selected, specfem2d writes an output file for each trace. When the SU format is selected, specfem2d writes one output file containing the data for the entire shot gather.

As you can imagine, this issue goes a bit deeper than causing some of the muting functions not to work. Other issues spawning from the number of output files generated also occur throughout the preprocess module, including in how the adjoint traces are written.

Because the difference in how seismograms are output is so significant depending on whether the ASCII or SU format is used in specfem2d, I wonder if it might actually be easier to just stick with the ASCII format and try to get the muting functions working that way. What do you think?

bch0w commented 11 months ago

Thanks @Ben-J-Eppinger, that's a great summary of the issue and will help with writing in an SU format reader/writer. The nice thing about SeisFlows is it's modularity, which allows us to easily write different interfaces for the 2D and 3D codes.

I think it is worthwhile to provide SU support, since it's useful for exploration scale problems, and will make SeisFlows stronger overall. In parallel I/we can work on getting those preprocessing functions working again, which should be straightforward bugfixing since the code is (mostly) written.

Let me start a new issue and try to outline the required tasks for a pull request that does a bit of overhaul on the preprocessing module. I will try to chip away at this over the next few weeks!

bch0w commented 3 months ago

Hi @Ben-J-Eppinger, finally finding time to get back to old issues and this is a big one to fix. I'm closing this issue in favor of #174 and hopefully will make some progress on this soon. Thanks again for the outline in the above comments.