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

Bugfix for r_power_n_segmented branch #63

Merged

Conversation

krisvanneste
Copy link
Collaborator

Claudio,

I fixed the bug which I introduced in the r_power_n_segmented branch.
In order to catch the case where hinge_distances is None, I had written:

hinge_distances or []

which implies a comparison, which is not supported if hinge_distances is an array.

Now that this is fixed, I would like to ask your advice concerning the current design:
Currently, hinge_distances (corresponding to geom_spread_n_distances configuration parameter) only contains the distances separating the different segments, with the smallest allowed distance hard-coded to 1.
Do you agree with this or would you prefer to include the smallest distance in the list?

@krisvanneste
Copy link
Collaborator Author

Is it possible I have no longer write access to this branch after a pull reqest?
I wanted to push a commit to remove the brackets from the geom_spread_n_exponents and geom_spread_n_distances example in the comments in config_files/configspec.conf, but access is denied.

@claudiodsf
Copy link
Member

Is it possible I have no longer write access to this branch after a pull reqest? I wanted to push a commit to remove the brackets from the geom_spread_n_exponents and geom_spread_n_distances example in the comments in config_files/configspec.conf, but access is denied.

That's bizarre. You should retain the write access. Did you try with a force push?

@krisvanneste
Copy link
Collaborator Author

Sorry, again my mistake. I was pushing to the wrong remote...

@claudiodsf
Copy link
Member

Nice ! I'll look into details later today

@claudiodsf
Copy link
Member

Now that this is fixed, I would like to ask your advice concerning the current design:
Currently, hinge_distances (corresponding to geom_spread_n_distances configuration parameter) only contains the distances separating the different segments, with the smallest allowed distance hard-coded to 1.
Do you agree with this or would you prefer to include the smallest distance in the list?

Why not allow the user to choose the smallest one? I would do something like this (where I also clarified that those are hypocentral distances):

# Exponents and hypocentral distances (in km) separating segments with different exponents
# for the "r_power_n_segmented" geometrical spreading function.
# Note that the smallest distance is generally set to 1, so that at small distances
# the attenuation is the same as "r_power_n".
# Example:
#   geom_spread_n_exponents = 1., 0., 0.5
#   geom_spread_n_distances = 1., 70, 130
geom_spread_n_exponents = float_list(min=0, default=None)
geom_spread_n_distances = float_list(min=0, default=None)

Of course, the check on the length of geom_spread_n_exponents and geom_spread_n_distances should be modified: they must have the same length now.

What do you think?

@krisvanneste
Copy link
Collaborator Author

I can agree with this implementation, but actually the purpose of the smallest distance is to have no further amplification at smaller distances. This also requires a modification in the geom_spread_r_power_n_segmented function. We need to replace

hinge_distances = np.hstack([[Rref], hinge_distances])

with

Rref = hinge_distances[0]

@claudiodsf
Copy link
Member

Ok. I just quickly scanned the Boore (2003) paper, so feel free to propose a better wording for the inline help in the config file.

I'll let you do the required commits. Let me know when it's ready to merge 😉

@krisvanneste
Copy link
Collaborator Author

OK

… function to correspond to start of each segment instead of distances separating segments.
…spreading model in _geometrical_spreading_coefficient function.
…'geom_spread_n_distances' configuration parameters for segmented geometrical spreading model.
# The number of exponents must be equal to the number of distances.
# Distances correspond to the start of each segment.
# Note that no geometrical spreading correction is considered
# below the smallest distance. This distance is generally set to 1.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe write "1 km" ?

@krisvanneste
Copy link
Collaborator Author

OK, will do.

@krisvanneste
Copy link
Collaborator Author

Done.
The behaviour below the smallest distance is not explicitly mentioned in the paper, but I simplified it from the Fortran code of SMSIM/EXSIM.

@claudiodsf
Copy link
Member

Great.

Just a curiosity: setting the smallest distance to 1 km isn't also a way to have a "classical" $1 / R^n$ attenuation law in the first segment? (as you were showing in the figure in the previous PR)

@krisvanneste
Copy link
Collaborator Author

krisvanneste commented Jan 9, 2025

Not per se.
Classical $1 / R^n$ attenuation is achieved by specifying 1 segment, i.e. 1 exponent and 1 distance.
However, the 1st distance also limits the spreading correction to the distances above it (because $1 / R^n$ attenuation for distances < 1 is actually amplification). So, for distances below the 1st distance, there will be a difference between r_power_n and r_power_n_segmented, even if the exponents are the same.

@claudiodsf
Copy link
Member

Ok, clear, thanks.

P.S. You can write LaTeX equations by putting them between $ ... $

@claudiodsf
Copy link
Member

It's good to go for me. Ok to merge it?

@krisvanneste
Copy link
Collaborator Author

Yes

@claudiodsf claudiodsf merged commit 8634634 into SeismicSource:main Jan 9, 2025
2 checks passed
@krisvanneste
Copy link
Collaborator Author

Switching back to main

@claudiodsf
Copy link
Member

Switching back to main

I'm also rebasing v2, I'll let you knwo when it's done

@claudiodsf
Copy link
Member

v2 is rebased and force-pushed. Could you check that this branch also works correctly for you?

@krisvanneste
Copy link
Collaborator Author

v2 is rebased and force-pushed. Could you check that this branch also works correctly for you?

OK. I haven't been working on v2 for a while, but I will try. I guess I will have to force-pull?

@claudiodsf
Copy link
Member

I guess I will have to force-pull?

Yes, and there's absolutely no hurry to test v2 😉

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.

2 participants