Skip to content

solar diag time series segfault - package needs variables#1228

Merged
kkeene44 merged 1 commit intowrf-model:release-v4.2.1from
davegill:time_series
Jul 10, 2020
Merged

solar diag time series segfault - package needs variables#1228
kkeene44 merged 1 commit intowrf-model:release-v4.2.1from
davegill:time_series

Conversation

@davegill
Copy link
Contributor

@davegill davegill commented Jun 26, 2020

TYPE: bug fix

KEYWORDS: process_time_series, solar, segfault

SOURCE: Franciano Puhales (Federal University of Santa Maria, Brazil) and internal

DESCRIPTION OF CHANGES:
When using time series with the solar diagnostic option activated, the model segfaults.
This was traced to missing fields in the package for tseries_add_solar: ts_p_profile
and ts_w_profile.

ISSUE:
Fixes #1227 (time series + solar diagnostics = seg fault)

LIST OF MODIFIED FILES:
modified: Registry.EM_COMMON

TESTS CONDUCTED:

  1. OP reports this fixes the issue.
  2. Jenkins is all PASS.

RELEASE NOTE: When using time series with the solar diagnostic option activated, the model segfaulted. This problem was traced to missing fields in the package for tseries_add_solar: ts_p_profile and ts_w_profile. The missing fields have been added to the package list, allowing the proper functioning of time series with solar diagnostics.

TYPE: bug fix

KEYWORDS: process_time_series, solar, segfault

SOURCE: Franciano Puhales () and internal

DESCRIPTION OF CHANGES:
When using time series with the solar diagnostic option activated, the model segfaults.
This was traced to missing fields in the package for `tseries_add_solar`: `ts_p_profile`
and `ts_w_profile`.

ISSUE:
Fixes wrf-model#1227 (time series + solar diagnostics = seg fault)

LIST OF MODIFIED FILES:
modified:   Registry.EM_COMMON

TESTS CONDUCTED:
1. OP who posted issue will report on status.
2. We'll see what jenkins thinks.

RELEASE NOTE: When using time series with the solar diagnostic option activated, the model segfaulted.  This problem was traced to missing fields in the package for tseries_add_solar: ts_p_profile and ts_w_profile. The fields have been added to the package list, allowing time series with solar diagnostics.
@davegill davegill requested a review from a team as a code owner June 26, 2020 02:32
@davegill
Copy link
Contributor Author

@fpuhales
Franciano,

  1. Please verify that this PR fixes your problem.
  2. Provide your affiliation for our release notice.

@davegill
Copy link
Contributor Author

Jenkins is OK

Please find result of the WRF regression test cases in the attachment. This build is for Commit ID: b3bca32adb5babd9290f043400f76ef09ad70e71, requested by: davegill for PR: https://github.com/wrf-model/WRF/pull/1228. For any query please send e-mail to David Gill.

    Test Type              | Expected  | Received |  Failed
    = = = = = = = = = = = = = = = = = = = = = = = =  = = = =
    Number of Tests        : 19           18
    Number of Builds       : 48           46
    Number of Simulations  : 166           164        0
    Number of Comparisons  : 105           104        0

    Failed Simulations are: 
    None
    Which comparisons are not bit-for-bit: 
    None

@fpuhales
Copy link

I did more tests here, and it seems to fix that issue. This PR fixes the problem. My affiliation is Atmospheric Modelling Group (GruMA), Federal University of Santa Maria (UFSM), Brazil.

Thanks, guys.

Copy link
Collaborator

@dudhia dudhia left a comment

Choose a reason for hiding this comment

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

looks fine - thanks Franciano

@weiwangncar
Copy link
Collaborator

This fix should be ok. It was Pedro who pointed out that ts_p_profile and ts_w_profile were missing from processing_time_series = 1. I guess when option 2 was added, they didn't add those two at the time.

@pedro-jm
Copy link
Contributor

@davegill
Dave,

This is the proper fix.

I think what happened is that when we found the bug with the p and w profiles time series we fixed the bug by adding p and w profiles to the regular time series package. I forgot to extend the fix to the solar diagnostic package like this PR is doing.

Any change in the standard time series package should be added to the solar diagnostic package in future developments to avoid this kind of issue.

Pedro.

@davegill
Copy link
Contributor Author

@pedro-jm @weiwangncar @dudhia @fpuhales
Thanks everyone!
Enjoy your Friday

@kkeene44 kkeene44 merged commit fa2d554 into wrf-model:release-v4.2.1 Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants