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 a tag column to the hera_obs table #618

Merged
merged 8 commits into from
Sep 8, 2022
Merged

Add a tag column to the hera_obs table #618

merged 8 commits into from
Sep 8, 2022

Conversation

bhazelton
Copy link
Member

Description

Add a tag column to the hera_obs table.

Note: As currently written, the create classmethod on the Observation object and the add_obs method on MCSession requires tag to be passed. So we will need to update RTP to pass it. The column is actually allowed to be null in the database, though, because all the old data do not have tags. I think long term we want RTP to set the tag field, but if it would be easier short term I can make that an optional parameter.

Motivation and Context

closes #605

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Schema change (any change to the SQL tables)
  • New feature without schema change (non-breaking change which adds functionality)
  • Change associated with a change in redis structure
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Version change
  • Build or continuous integration change
  • Other

Checklist:

Schema change:

  • My code follows the code style of this project.
  • I have added or updated the docstrings associated with my feature using the numpy docstring format.
  • I have created an alembic version file to produce the schema change.
  • I have tested looping upgrading and downgrading the alembic version and tests pass consistently.
  • I understand the updates required onsite (detailed in the readme) and I will make those
    changes when this is merged.
  • I have updated the schema documentation document with my changes (under docs/mc_definition.tex)
  • I have added tests to cover my new feature.
  • Unit tests pass on site (This is a critical check, CI can differ from site).
  • I have updated the CHANGELOG.

@bhazelton bhazelton requested a review from plaplant August 8, 2022 19:54
@codecov
Copy link

codecov bot commented Aug 8, 2022

Codecov Report

Merging #618 (308e027) into main (b9a2bf3) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #618      +/-   ##
==========================================
+ Coverage   98.14%   98.22%   +0.08%     
==========================================
  Files          35       35              
  Lines        5123     5130       +7     
==========================================
+ Hits         5028     5039      +11     
+ Misses         95       91       -4     
Impacted Files Coverage Δ
hera_mc/mc.py 89.25% <100.00%> (+2.47%) ⬆️
hera_mc/mc_session.py 99.76% <100.00%> (+0.11%) ⬆️
hera_mc/observations.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@plaplant plaplant left a comment

Choose a reason for hiding this comment

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

Thanks @bhazelton! I think this looks great. I just have a question and found a few print statements that should be removed.

We should also change mc_add_observation.py to add the tag to the observation when it's first added to M&C. I can make that change as part of this PR.

hera_mc/tests/test_observations.py Outdated Show resolved Hide resolved
hera_mc/tests/test_observations.py Outdated Show resolved Hide resolved
hera_mc/observations.py Show resolved Hide resolved
Copy link
Member

@plaplant plaplant left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @bhazelton! Merging this in so we can do a rebase for the forthcoming PR.

@plaplant plaplant merged commit 5cda992 into main Sep 8, 2022
@plaplant plaplant deleted the add_tag_obs_table branch September 8, 2022 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add observing tag to the hera_obs table
2 participants