Skip to content

Conversation

@antonio-scala
Copy link

After pushing to my fork, some workflow actions fail.
I assume this is related to running the workflows in a forked repository and should not affect the pull request.
If not, I would appreciate any suggestions on how to address the issue.

Copy link
Contributor

@CB-quakemodel CB-quakemodel left a comment

Choose a reason for hiding this comment

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

Dear Antonio,

Many thanks for submitting your GMM in an OQ implementation.

I have made some small comments throughout the new files provided in your PR.

I think it would be advisable to incorporate all of the subclasses into a single python module (i.e. place the moment magnitude and duration magnitude subclasses into a single .py file, rather than having separate .py files for each - correspondingly the unit test files could be reduced by following this approach). This would make sense from a gsim library management perspective given it will keep the number of python modules down, and these GMMs belong to the same publication.

Many thanks,

Christopher

"""
Compute the distance function
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

Spurious space

Test tables are generated from a spreadsheet provided by the authors, and
modified according to OQ format (e.g. conversion from cm/s2 to m/s2).
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

Spurious space

<.base.GroundShakingIntensityModel.compute>`
for spec of input and result values.
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

Spurious spaces here


for m, imt in enumerate(imts):
C = self.COEFFS[imt]
rval_distance = getattr(ctx, self.DIST_TYPE)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be cleaner I think to retrieve the dist type from self.REQUIRES_DISTANCES given the information is the same as that you are storing in self.DIST_TYPE

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