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

Add handling of "Average" and "Alt_Average" data types to Signature #229

Merged
merged 7 commits into from
May 23, 2024

Conversation

ssuttles-usgs
Copy link
Collaborator

Add handling of "Average" and "Alt_Average" data types to Signature processing. Additionally, had creation of instrument transformation matrix and use time step to determine time encoding instead of non-presences of "sample" dimension in data set. Test for new data type(s) added to test scripts.

Copy link
Member

@dnowacki-usgs dnowacki-usgs left a comment

Choose a reason for hiding this comment

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

Thanks Steve! A few rather minor comments

print(
"make time encoding to dtype double because no sample dimension, round to milliseconds first"
f"make time encoding to dtype double because tstep {tstep} seconds is < 1 minute, round to milliseconds first"
Copy link
Member

Choose a reason for hiding this comment

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

Rather than printing this to the screen, please add it to the netCDF history using utils.insert_history (that will also print it to the screen).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed as suggested.

)
ds["time"] = ds["time"].dt.round(
"s"
) # round time to seconds if time interval >= 1 minute
Copy link
Member

Choose a reason for hiding this comment

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

Please put comments like this on their own line so black doesn't do this kind of unnecessary line wrapping

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed as suggested.

)
ds["time"] = ds["time"].dt.round(
"s"
) # round time to seconds if time interval >= 1 minute
ds["time"].encoding["dtype"] = "i4"
Copy link
Member

Choose a reason for hiding this comment

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

We should make sure this doesn't cause an overflow. Please add a check using utils.check_fits_in_int32

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add time handling to utils.check_fits_in_int32 and added check for dtype 'i4' time encoding.

.gitattributes Outdated
@@ -4,3 +4,5 @@ stglib/_version.py export-subst
stglib/tests/data/055109_20220808_1605.zip filter=lfs diff=lfs merge=lfs -text
stglib/tests/data/055109_20220808_1605_config.yaml filter=lfs diff=lfs merge=lfs -text
stglib/tests/data/gatts_055109_20220808_1605.txt filter=lfs diff=lfs merge=lfs -text
stglib/tests/data/*.cdf filter=lfs diff=lfs merge=lfs -text
Copy link
Member

Choose a reason for hiding this comment

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

Ideally the test files should be listed in the .gitattributes file in stglib/tests/data, not this .gitattributes file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved these lines to .gitattributes file in stglib/tests/data folder

Copy link
Member

@dnowacki-usgs dnowacki-usgs left a comment

Choose a reason for hiding this comment

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

Thanks Steve. Just a couple more things.

f"32-bit integer overflow on {var}; setting encoding to i4 will fail"
)
return False
def check_fits_in_int32(ds, var, units="s"):
Copy link
Member

Choose a reason for hiding this comment

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

Whoops, sorry, I could have been more clear. There is already a check_time_fits_in_int32 function, defined below. Please use that one for time rather than changing this func def (unless there is something else I am missing).

.gitattributes Outdated
stglib/tests/data/055109_20220808_1605.zip filter=lfs diff=lfs merge=lfs -text
stglib/tests/data/055109_20220808_1605_config.yaml filter=lfs diff=lfs merge=lfs -text
stglib/tests/data/gatts_055109_20220808_1605.txt filter=lfs diff=lfs merge=lfs -text
stglib/_version.py export-subst
Copy link
Member

Choose a reason for hiding this comment

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

Please add a newline at the end of this file.

@dnowacki-usgs dnowacki-usgs changed the title Sig updates avgd Add handling of "Average" and "Alt_Average" data types to Signature May 23, 2024
@dnowacki-usgs dnowacki-usgs merged commit cb9e559 into USGS-CMG:master May 23, 2024
11 checks passed
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