Conversation
Summary of ChangesHello @lispandfound, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive suite of new plotting tools and significantly enhances existing visualization capabilities. The primary goal is to provide more robust and versatile methods for analyzing and presenting seismic simulation outputs, particularly in comparison with empirical Ground Motion Models. Key improvements include advanced 3D ground motion animations, detailed GMM comparison plots, and the integration of R for statistical data fitting. These changes streamline the process of generating high-quality visualizations and facilitate deeper scientific analysis of simulation results. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several new plotting scripts for visualizing simulation results, including GMM comparisons, response spectra, FAS, and waveforms. It also refactors existing plotting scripts to improve code quality and add new features. Review comments highlight several issues, including hardcoded paths and values, type mismatches, incorrect calculations, incomplete documentation, and deprecated Matplotlib arguments. The reviewer suggests making paths and parameters configurable, correcting type mismatches, using named constants for magic numbers, completing docstrings, and ensuring compatibility with older Python versions.
| lines.extend([point_a, point_b]) | ||
|
|
||
| cmap_min, cmap_max, cmap = cmap_from_cpt( | ||
| Path("/home/jake/tmp/palm_springs_nz_topo.cpt") |
There was a problem hiding this comment.
The path "/home/jake/tmp/palm_springs_nz_topo.cpt" is hardcoded and points to a specific user's temporary directory. This makes the code non-portable and will cause failures on other systems. This path must be made configurable, either as an argument or a setting, and should ideally point to a resource within the project or a well-defined location.
# TODO: Make CPT file path configurable
cmap_min, cmap_max, cmap = cmap_from_cpt(
Path("/path/to/your/cpt/file.cpt")
)| ) -> None: | ||
| freqs = dataset.frequency.values | ||
| vs30 = dataset.vs30.sel(station=station).item() | ||
| site_properties = utils.compute_site_properties(vs30) |
There was a problem hiding this comment.
The utils.compute_site_properties function expects an xr.Dataset but is being called with a single vs30 float. This is a type mismatch and will cause a runtime error. Please adjust the call to pass the entire site xarray dataset, or modify utils.compute_site_properties to accept a float if that's the intended usage here.
| site_properties = utils.compute_site_properties(vs30) | |
| site_properties = utils.compute_site_properties(site) |
| avg_rake = np.degrees(circmean(np.radians(rakes), np.array(moments))) | ||
| avg_dip = float(np.average(dips, weights=moments)) | ||
| avg_moment = float(np.mean(moments)) | ||
| total_moment = avg_moment * len(moments) |
There was a problem hiding this comment.
The calculation total_moment = avg_moment * len(moments) is incorrect. If avg_moment is already the mean of the individual moments, then total_moment should be the sum of the moments, which is np.sum(moments). Multiplying the average by the count of moments will give the sum, but it's more direct and clearer to use np.sum if moments is a list/array of individual moments.
| total_moment = avg_moment * len(moments) | |
| total_moment = np.sum(moments) |
| def compute_site_properties(sites: xr.Dataset) -> SiteProperties: | ||
| vs30 = sites.vs30.values | ||
| z1pt0 = oqw.estimations.chiou_young_08_calc_z1p0(vs30) | ||
| z2pt5 = oqw.estimations.chiou_young_08_calc_z2p5(vs30) | ||
| rrup = sites.rrup.values | ||
| rjb = sites.rjb.values | ||
| rx = rjb | ||
| ry = rjb | ||
| vs30measured = False | ||
| return SiteProperties( | ||
| vs30measured=vs30measured, | ||
| vs30=vs30, | ||
| z1pt0=z1pt0, | ||
| z2pt5=z2pt5, | ||
| rrup=rrup, | ||
| rjb=rjb, | ||
| rx=rx, | ||
| ry=ry, | ||
| ) |
There was a problem hiding this comment.
The compute_site_properties function is designed to take an xr.Dataset as input, but it appears to be called with a single vs30 float in visualisation/waveforms/plot_fas.py. This is a type mismatch that will lead to a runtime error. The function either needs to be refactored to accept a single vs30 value, or the calling code needs to pass an xr.Dataset containing the site properties.
def compute_site_properties(
vs30: float | npt.NDArray[np.floating],
rrup: float | npt.NDArray[np.floating],
rjb: float | npt.NDArray[np.floating],
) -> SiteProperties:
vs30_val = vs30 if isinstance(vs30, (float, np.floating)) else vs30.item()
z1pt0 = oqw.estimations.chiou_young_08_calc_z1p0(vs30_val)
z2pt5 = oqw.estimations.chiou_young_08_calc_z2p5(vs30_val)
rx = rjb
ry = rjb
vs30measured = False
return SiteProperties(
vs30measured=vs30measured,
vs30=vs30,
z1pt0=z1pt0,
z2pt5=z2pt5,
rrup=rrup,
rjb=rjb,
rx=rx,
ry=ry,
)| # Write the raw video data to FFmpeg's stdin | ||
| for frame in frames: | ||
| process.stdin.write(frame) | ||
| scalars[scalars < 10] = np.nan |
There was a problem hiding this comment.
The value 10 is a magic number. Please define it as a named constant with a clear explanation of its purpose (e.g., MIN_SCALAR_THRESHOLD) to improve readability and maintainability.
MIN_SCALAR_THRESHOLD = 10
scalars[scalars < MIN_SCALAR_THRESHOLD] = np.nan| ax.set_xlim(freqs.min(), 50) | ||
| ax.set_xscale("log") | ||
| ax.set_yscale("log") | ||
| ax.grid(visible=True, which="both", axis="both", lw=0.3) |
There was a problem hiding this comment.
The visible argument for ax.grid() is deprecated in Matplotlib 3.4 and later. Please remove visible=True as True is the default behavior for ax.grid().
| ax.grid(visible=True, which="both", axis="both", lw=0.3) | |
| ax.grid(which="both", axis="both", lw=0.3) |
|
|
||
|
|
||
| def find_region(domain: DomainParameters) -> tuple[float, float, float, float]: | ||
| """Find an appropriate domain,""" |
There was a problem hiding this comment.
The docstring for find_region is incomplete. It ends with a comma, suggesting that the description is unfinished. Please complete the docstring to clearly explain what the function does.
| """Find an appropriate domain,""" | |
| def find_region(domain: DomainParameters) -> tuple[float, float, float, float]: | |
| """Find an appropriate domain for plotting.""" |
| / 1000 | ||
| ) | ||
| rupture_df = pd.DataFrame( | ||
| { |
There was a problem hiding this comment.
The value vs30measured=False is hardcoded. If this value can vary, it should be derived from the input data or made a configurable parameter. If it's always False for this context, a comment explaining why would be beneficial.
"vs30measured": False, # Assuming vs30 is not measured for this context| ax.set_xlabel("Period [s]") | ||
| ax.set_xscale("log") | ||
| ax.set_yscale("log") | ||
| ax.grid(visible=True, which="both", axis="both", lw=0.3) |
There was a problem hiding this comment.
The visible argument for ax.grid() is deprecated in Matplotlib 3.4 and later. Please remove visible=True as True is the default behavior for ax.grid().
| ax.grid(visible=True, which="both", axis="both", lw=0.3) | |
| ax.grid(which="both", axis="both", lw=0.3) |
| CMS2 = "cm/s^2" | ||
|
|
||
|
|
||
| _UNIT_CONVERSION_TABLE = {Units.G: 981.0, Units.CMS: 1.0, Units.CMS2: 1.0} |
There was a problem hiding this comment.
The conversion factor 981.0 for Units.G is hardcoded. While this is the standard acceleration due to gravity in cm/s², it's a magic number. Consider defining it as a named constant (e.g., GRAVITY_CMS2) to improve readability and maintainability.
| _UNIT_CONVERSION_TABLE = {Units.G: 981.0, Units.CMS: 1.0, Units.CMS2: 1.0} | |
| GRAVITY_CMS2 = 981.0 | |
| _UNIT_CONVERSION_TABLE = {Units.G: GRAVITY_CMS2, Units.CMS: 1.0, Units.CMS2: 1.0} |
1 participant
No description provided.