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

DM-24283: Update nullable fields #164

Merged
merged 1 commit into from Jan 3, 2024
Merged

DM-24283: Update nullable fields #164

merged 1 commit into from Jan 3, 2024

Conversation

bsmartradio
Copy link
Contributor

Change several fields from non-nullable to nullable

@bsmartradio bsmartradio force-pushed the tickets/DM-24283 branch 2 times, most recently from b60096b to 3833003 Compare November 23, 2023 00:00
Copy link
Collaborator

@JeremyMcCormick JeremyMcCormick left a comment

Choose a reason for hiding this comment

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

Please delete the .idea files that were added (presumably accidentally). They don't look like they belong in the repo.

The branch also needs an "Update with rebase," too.

@bsmartradio bsmartradio force-pushed the tickets/DM-24283 branch 2 times, most recently from 20e2c42 to 42e2c87 Compare November 30, 2023 20:20
Copy link
Collaborator

@JeremyMcCormick JeremyMcCormick left a comment

Choose a reason for hiding this comment

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

The commit history was fixed to take out the erroneous files and rebase was performed. Looks okay now.

Copy link
Collaborator

@gpdf gpdf left a comment

Choose a reason for hiding this comment

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

I'm sorry for perhaps being fussy, but I'm concerned that the commit message buries the lede, as it were. An at least equally important part of this PR is changing "Sigma" to "Err" in a number of places. Because it's not mentioned in the commit message it leaves some question as to whether it's intentional (or, say, an inadvertent regression).

Is this a remnant of work triggered by RFC-333?

The ticket reference in the PR title, to DM-23283, appears to be to something unrelated, a change to the handling of airmass. The branch name, including DM-24283, is about Avro schema documentation. Neither Jira ticket mentions the Sigma/Err change.

Could you update the commit message to clarify this, please?

There are also attribute names like DiaObject.u_scienceFluxSigma still in apdb.yaml. Should those be changed as well?

Finally, I note that identically named DiaObject fields in imsim.yaml are still in the "FluxSigma" form. Might it be preferable to change them at the same time?

Thank you!
-Gregory

@ebellm
Copy link
Contributor

ebellm commented Dec 1, 2023

@bsmartradio Can you comment on why Sigma changed to Err here? This change happened in the force push after my review, it looks like. Reading https://jira.lsstcorp.org/browse/RFC-333 I believe that that the original Sigma is correct, as we're characterizing the width of the distribution of DIASource fluxes associated with the DIAObject.

@ebellm ebellm changed the title DM-23283: Update nullable fields DM-24283: Update nullable fields Dec 1, 2023
@ebellm
Copy link
Contributor

ebellm commented Dec 1, 2023

@gpdf I updated the PR title to match the ticket number. This change came up in the process of updating our Avro tools to build schemas directly from the sdm_schemas yaml source of truth. We identified a few fields that were not allowed to be null that needed to be; that should be the only change happening on this ticket.

@bsmartradio bsmartradio force-pushed the tickets/DM-24283 branch 2 times, most recently from 6d2fcc8 to 9593d51 Compare December 4, 2023 13:13
@ebellm ebellm requested a review from gpdf December 4, 2023 17:53
Copy link
Collaborator

@gpdf gpdf left a comment

Choose a reason for hiding this comment

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

This seems to be appropriately limited to the stated goals now.

Please be aware that there is at least one other approved pending pull request likely to be merged today, so please check that main hasn't changed, and then rebase accordingly if needed, before merging.

@gpdf
Copy link
Collaborator

gpdf commented Dec 5, 2023

I'd encourage rebasing and merging this soon, as it seems it's fully signed-off - we're trying to clear a backlog of PRs on this repo.

@bsmartradio
Copy link
Contributor Author

I'd encourage rebasing and merging this soon, as it seems it's fully signed-off - we're trying to clear a backlog of PRs on this repo.

There is one lingering issue that only shows up in ap_verify which I am trying to track down. I think I will have it sorted today.

@gpdf
Copy link
Collaborator

gpdf commented Dec 5, 2023

OK, no worries. If it's still in-work that's fine!

@bsmartradio bsmartradio force-pushed the tickets/DM-24283 branch 2 times, most recently from d5e86e4 to ae0d7ea Compare December 14, 2023 19:29
@JeremyMcCormick JeremyMcCormick marked this pull request as draft December 20, 2023 22:48
@JeremyMcCormick
Copy link
Collaborator

I've marked this as a draft so it doesn't get unintentionally merged as it has been approved already, but there was an indication by @bsmartradio that work still needs to be done on it.

@bsmartradio bsmartradio marked this pull request as ready for review December 22, 2023 23:30
@bsmartradio bsmartradio merged commit 2796f74 into main Jan 3, 2024
4 checks passed
@bsmartradio bsmartradio deleted the tickets/DM-24283 branch January 3, 2024 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants