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

Fix test reference mismatch #786

Merged
merged 12 commits into from
Dec 19, 2023
Merged

Fix test reference mismatch #786

merged 12 commits into from
Dec 19, 2023

Conversation

apchoiCMD
Copy link
Collaborator

@apchoiCMD apchoiCMD commented Dec 1, 2023

What this PR fixed

  • Two new ioda converters that work with mktime in a C library function that need to ensure consistent behavior across different machines with different time zones (e.g., Orion (CST) and Hera (UTC))
  • Set the time zone explicitly using the tzset function with UTC time zone
  • Used oops/utils to make dateime to Julian date so now code is a lot simpler than before

Fixed #783

Copy link
Contributor

@guillaumevernieres guillaumevernieres left a comment

Choose a reason for hiding this comment

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

I don't think changing the environment is a safe thing to do, you're assuming TZ isn't used anywhere else.

utils/obsproc/IcecAmsr2Ioda.h Outdated Show resolved Hide resolved
Copy link
Contributor

@guillaumevernieres guillaumevernieres left a comment

Choose a reason for hiding this comment

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

Have a look here @apchoiCMD :
https://github.com/JCSDA/ioda/blob/develop/src/core/IodaUtils.h

Copy link
Contributor

@guillaumevernieres guillaumevernieres left a comment

Choose a reason for hiding this comment

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

Thanks @apchoiCMD . Looks good, just a few minor comments. Did you check that the datetime is still correct?

utils/obsproc/IcecAmsr2Ioda.h Outdated Show resolved Hide resolved
utils/obsproc/IcecAmsr2Ioda.h Outdated Show resolved Hide resolved
utils/obsproc/IcecAmsr2Ioda.h Outdated Show resolved Hide resolved
utils/obsproc/Smap2Ioda.h Outdated Show resolved Hide resolved
@apchoiCMD
Copy link
Collaborator Author

Thanks @apchoiCMD . Looks good, just a few minor comments. Did you check that the datetime is still correct?

Yes, I have checked min & max output by using online tools (UTC) compared to original netcdf data, too! Everything was ok!

Copy link
Collaborator

@AndrewEichmann-NOAA AndrewEichmann-NOAA left a comment

Choose a reason for hiding this comment

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

Aside from the filtering of obs with bad dates, looks good

@guillaumevernieres guillaumevernieres added the hera-GW-RT Queue for automated testing with global-workflow on Hera label Dec 19, 2023
Copy link
Contributor

@guillaumevernieres guillaumevernieres left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @apchoiCMD .

@emcbot emcbot added hera-GW-RT-Running Automated testing with global-workflow running on Hera and removed hera-GW-RT Queue for automated testing with global-workflow on Hera labels Dec 19, 2023
Copy link
Collaborator

@ShastriPaturi ShastriPaturi left a comment

Choose a reason for hiding this comment

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

Approving based on others comments

@emcbot
Copy link

emcbot commented Dec 19, 2023

Automated Global-Workflow GDASApp Testing Results:
Machine: hera

Start: Tue Dec 19 20:35:00 UTC 2023 on hfe07
---------------------------------------------------
Build:                                 *SUCCESS*
Build: Completed at Tue Dec 19 21:29:29 UTC 2023
---------------------------------------------------
Tests:                                  *Failed*
Tests: Failed at Tue Dec 19 21:47:46 UTC 2023
Tests: 87% tests passed, 7 tests failed out of 55
	1701 - test_gdasapp_soca_JGDAS_GLOBAL_OCEAN_ANALYSIS_RUN (Failed)
	1703 - test_gdasapp_soca_JGDAS_GLOBAL_OCEAN_ANALYSIS_CHKPT (Failed)
	1704 - test_gdasapp_soca_JGDAS_GLOBAL_OCEAN_ANALYSIS_POST (Failed)
	1705 - test_gdasapp_soca_JGDAS_GLOBAL_OCEAN_ANALYSIS_VRFY (Failed)
	1707 - test_gdasapp_soca_incr_handler (Failed)
	1719 - test_gdasapp_atm_jjob_var_run (Failed)
	1720 - test_gdasapp_atm_jjob_var_final (Failed)
Tests: see output at /scratch1/NCEPDEV/da/Cory.R.Martin/CI/GDASApp/workflow/PR/786/global-workflow/sorc/gdas.cd/build/log.ctest

@emcbot emcbot added hera-GW-RT-Failed Automated testing with global-workflow failed on Hera and removed hera-GW-RT-Running Automated testing with global-workflow running on Hera labels Dec 19, 2023
@guillaumevernieres guillaumevernieres merged commit 146c790 into develop Dec 19, 2023
5 checks passed
@apchoiCMD apchoiCMD deleted the bugfix/ref-mismatch branch February 5, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hera-GW-RT-Failed Automated testing with global-workflow failed on Hera
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test reference mismatch
5 participants