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

feature update: instrument and satellite meta tables for arrays #39

Merged
merged 21 commits into from
May 13, 2024

Conversation

jrknezha
Copy link
Collaborator

This PR adjusts how satellite and instrument information is stored and related to array metrics and types. Previously, we had a single satellite meta down to the channel associated with a type, this was too restrictive. This PR splits this information into two tables sat_meta (sat id, name, short name) and instrument meta (name, num channels, scan angle) and associates them separately between the array metric type (instrument) and array metric values (specific sat).

This will extend our ability to use the satellite and instrument in flexible ways for arrays. In the future, these tables can also be associated with the experiment metric and metric types tables for single values.

Modified files for the new table structures and relationships, handler files and testing, and added new files for instrument meta handler and tests.

I also commented out the harvest innov stats tests which is a known failure and complicated testing procedures (this should be addressed in future PRs and has been captured as issue #38)

@jrknezha
Copy link
Collaborator Author

need to add the updated tables to the readme

@jrknezha
Copy link
Collaborator Author

ReadME has been updated and the PR is fully ready for review

Copy link
Collaborator

@amschne amschne left a comment

Choose a reason for hiding this comment

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

Can reproduce passing unit tests. Noting here to also include in this PR a new namedtuple in src/harvest_translator.py that corresponds to the new expt_array_metrics table.

I provided a few minor (stylistic) comments to consider.

src/instrument_meta.py Outdated Show resolved Hide resolved
src/array_metric_types.py Show resolved Hide resolved
src/array_metric_types.py Outdated Show resolved Hide resolved
src/array_metric_types.py Show resolved Hide resolved
src/expt_array_metrics.py Show resolved Hide resolved
src/instrument_meta.py Show resolved Hide resolved
src/instrument_meta.py Show resolved Hide resolved
src/instrument_meta.py Show resolved Hide resolved
src/score_table_models.py Show resolved Hide resolved
tests/test_harvest_innov_stats_engine.py Show resolved Hide resolved
@jrknezha jrknezha self-assigned this May 10, 2024
src/harvest_metrics.py Outdated Show resolved Hide resolved
@jrknezha
Copy link
Collaborator Author

confirming at this point that all tests are running successfully with the new changes. since we don't have any harvesters set up for the array metrics, I can't write in a realistic test for the new harvest with is_array part

Copy link
Collaborator

@KevinCounts KevinCounts left a comment

Choose a reason for hiding this comment

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

All tests are passing for me, and everything else looks good. Double checked code and readme changes against the db schema changes as well. I think this is good to go.

@jrknezha jrknezha merged commit f0c313a into develop May 13, 2024
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.

3 participants