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

Provenance Tagging #338

Merged
merged 20 commits into from
Jul 31, 2024
Merged

Provenance Tagging #338

merged 20 commits into from
Jul 31, 2024

Conversation

rknop
Copy link
Contributor

@rknop rknop commented Jul 25, 2024

Human-readable tags associated with provenances. (Issue #337 )

@rknop rknop requested a review from whohensee July 30, 2024 14:08
Copy link
Collaborator

@whohensee whohensee left a comment

Choose a reason for hiding this comment

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

I think it looks good, no major issues found. It's nice to have this working, I think something feels very official about having the capability to have these version tags. Soon we will decide on the "launch" parameters...

objects = get_all_database_objects( session=dbsession )
any_objects = False
for Class, ids in objects.items():
# TODO: check that surviving provenances have test_parameter
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you want to do this in this PR or leave it as a todo?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see that this is a remnant TODO from a prior PR

# (This is probably not practical, becasuse there is *so much* module
# and session scope stuff that lots of things are left behind by tests.
# You will have to sift through a lot of output to find what you're
# looking for. We need a better way.)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Useful comment block, and I agree that we could use a better way down the line.

assert len(rows) == 7
# check stuff about the rows?

# There is probably more we should be testing here. Definitely.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can robustify these tests down the line I think, but these look like a good starting point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lots of stuff is going to be added to the webap, so tests will be added then.

)
subdict['provtag'] = data['provenancetag']
# I wonder if making a primary key on the temp table would be more efficient than
# all these columns in GROUP BY? Investigate this.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like I could take an entire day just studying this monstrous 3-part query! sqlalchemy has shielded me from pure SQL, mostly...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, these things get kind of nasty.

Adding the provenance tags made these queries much more complicated.

Comment on lines 202 to 203
{ "text": '"Detections" are everything found on subtratcions; ' +
'"Sources" are things that passed prelminary cuts.' } )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{ "text": '"Detections" are everything found on subtratcions; ' +
'"Sources" are things that passed prelminary cuts.' } )
{ "text": '"Detections" are everything found on subtractions; ' +
'"Sources" are things that passed preliminary cuts.' } )

Copy link
Collaborator

Choose a reason for hiding this comment

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

it wouldn't be a PR without a typo!

@rknop rknop marked this pull request as ready for review July 30, 2024 21:44
@rknop rknop merged commit 480c291 into c3-time-domain:main Jul 31, 2024
7 checks passed
@rknop rknop deleted the provtag branch September 5, 2024 17:20
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