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

SeismicPlotter scale options does not work #561

Open
pavlis opened this issue Sep 8, 2024 · 1 comment
Open

SeismicPlotter scale options does not work #561

pavlis opened this issue Sep 8, 2024 · 1 comment

Comments

@pavlis
Copy link
Collaborator

pavlis commented Sep 8, 2024

I noticed some time ago that the scale argument for the constructor of the SeismicPlotter does nothing. We have workarounds in the tutorials that use the scale algorithm before calling the plot method. I just looked and the cause of this bug is clear: it is a feature that the comments say was not completed. Here, in fact, is the current section that needs to be fixed:

    def _normalize(self, d):
        """
        Normalizes data (d) using scale function.  for ensembles that
        is peak normalization to level self.scale by section. For
        Seismograms each component will be normalized independently.
        For TimeSeries the peak is adjusted to self.scale.

        :param d:  input data to be scanned (must be one of mspass supported
        data objects or will throw an exception)
        :param perf:  clip level as used in seismic unix displays.
        """
        # These are place holders for now.  Requires some new code in ccore
        # to sort absolute values and return perf level - should use faster
        # max value when perf is 100%
        if isinstance(d, SeismogramEnsemble):
            alg_scale(d, scale_by_section=True, level=self.scale)
        elif isinstance(d, TimeSeriesEnsemble):
            alg_scale(d, scale_by_section=True, level=self.scale)
        elif isinstance(d, TimeSeries):
            alg_scale(d, level=self.scale)
        elif isinstance(d, Seismogram):
            alg_scale(d, level=self.scale)
        else:
            raise RuntimeError(
                "SeismicPlotter._normalize:  Received unsupported data type=", type(d)
            )

A simple fix is to replace that whole chain of if-elif-else conditions to this:

d=scale(d,level=self.scale,method='peak')

which would make the _normalize method pretty trivial. I didn't do this for two reasons:

  1. I think this shows a need to add a self.scaling_method to the SeismicPlotter class constructor that would allow the scaling to be run like this: d=scale(d,level=self.scale,method=self.scaling_method) . There are other options for scale, but the method is one that should be selectable as you can get very different results with different scaling methods. The "perc" algorithm as used in seismic unix, for example, is known to be useful. What we need really is a default set of arguments for scaling that would make most plots useful without options. Right now that is not true as it is too easy to get a black screen from the wrong scaling. So, I think the group needs to discuss what options we need before I jump in and just fix this crudely.
  2. We need a pytest function for this kind of thing in SeismicPlotter. There is currently no pytest for the graphics module, which is not good. I know no way to test if the graphics functionality in pytest, but we should at least test the exception handlers and functions like _normalize that don't produce graphical output.

An additional change we should make with this bug fix: I strongly recommend we change the default "style" from "wtvaimg" to "wt". It is faster and more conventional to most seismologists.

@pavlis
Copy link
Collaborator Author

pavlis commented Sep 15, 2024

I may have fixed this bug in pull request #558 but that needs to be verified before we close this. I know I fixed the issue with dead data but am less confident I fixed this one.

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

No branches or pull requests

1 participant