DM-41063: Update docstring for DiaSource.ext_trailedSources_Naive_flag_edge

[bsmartradio]

The docstring for DiaSource.ext_trailedSources_Naive_flag_edge has been updated to reflect that it only indicates whether or not a trailed source's endpoints contain edge pixels. Other flags pertaining to trailed sources will not be persisted at this time and are only used for filtering purposes in filterDiaSourceCatalog.py in ap_association.

[gpdf]

This is just a comment, not an objection.

These column names are starting to get very long, sometimes more than 50% of the length of the description. Long column names are not going to be super-popular with users creating ADQL queries, and they also don't lead to a great UX in a visualization, because either one displays the whole string -- with associated distortions of the UI, like very wide columns that only have 'T' or 'F' in them -- or some generic truncation of it, which might not be nearly as informative as a deliberately constructed shorter name.

Note that the description string is available in almost every context, Portal, PyVO, TOPCAT, etc.

I'm not requesting changes because we don't have a stated maximum length and we also haven't leaned hard on this in the past, but it would be good to think it through.

[JeremyMcCormick]

Please remove all of these mysql:datatype: BOOLEAN overrides (including the two below) as they are now redundant with datatype: boolean and will be flagged in the validation as errors in the near future.

[JeremyMcCormick]

@bsmartradio You should pull recent updates to main and rebase your branch. Otherwise you are going to see erroneous validation errors in some of the other schemas.

[gpdf]

@bsmartradio You should pull recent updates to main and rebase your branch. Otherwise you are going to see erroneous validation errors in some of the other schemas.

And it looks like you can ultimately squash the three commits that are currently on this branch.

[gpdf]

The current state of this PR appears to be that it only changes one line of code, the description for the column DiaSource.ext_trailedSources_Naive_flag_edge . Is that correct or is the branch still in flux?

[bsmartradio]

This is still in flux as the other 5 PR's related to it are still in the works. This will likely be the only change and this PR will be updated.

…

________________________________ From: gpdf ***@***.***> Sent: Monday, May 6, 2024 5:37 PM To: lsst/sdm_schemas ***@***.***> Cc: Brianna Smart ***@***.***>; Mention ***@***.***> Subject: Re: [lsst/sdm_schemas] DM-41063: Add nan, suspect_long_trial, and off_image flags (PR #191) The current state of this PR appears to be that it only changes one line of code, the description for the column DiaSource.ext_trailedSources_Naive_flag_edge . Is that correct or is the branch still in flux? — Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/lsst/sdm_schemas/pull/191*issuecomment-2097150550__;Iw!!K-Hz7m0Vt54!m5gzBcs6NysWnSWkipTnxv0K48G4YHUVPtXyTk5HgWsOaonYzrG4pzLkmt9u6UvYUG_nR0SN1k3Ebq8jP1ocupA$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AWZRE6HYW273E736JC4AIYDZBAO4TAVCNFSM6AAAAABEJ5ZQZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJXGE2TANJVGA__;!!K-Hz7m0Vt54!m5gzBcs6NysWnSWkipTnxv0K48G4YHUVPtXyTk5HgWsOaonYzrG4pzLkmt9u6UvYUG_nR0SN1k3Ebq8jvlEhf4A$>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[JeremyMcCormick]

This is still in flux as the other 5 PR's related to it are still in the works. This will likely be the only change and this PR will be updated.

This is such a minor change that it should just be rolled into some other update or put on a user branch. We only really merge ticket branches like this when they resolve the connected Jira issue, and we don't reuse branch names or generally allow merging the same branch more than once.

5 PR's related to it are still in the works

It would be my preference that these are split into separate Jira issues and ticket branches. For anything but trivial changes we aim for a one-to-one correspondence between PRs and tickets on this project. There are exceptions when applying a small patch/fix/update to an already merged branch (with branch naming like DM-12345-fixed-something) or making minor updates on a user branch, but generally this would be a good pattern to follow for organizational purposes. It is also crucial for generating easily understandable release notes where each item should correspond to a resolved ticket.

[bsmartradio]

This is still in flux as the other 5 PR's related to it are still in the works. This will likely be the only change and this PR will be updated.

This is such a minor change that it should just be rolled into some other update or put on a user branch. We only really merge ticket branches like this when they resolve the connected Jira issue, and we don't reuse branch names or generally allow merging the same branch more than once.

5 PR's related to it are still in the works

It would be my preference that these are split into separate Jira issues and ticket branches. For anything but trivial changes we aim for a one-to-one correspondence between PRs and tickets on this project. There are exceptions when applying a small patch/fix/update to an already merged branch (with branch naming like DM-12345-fixed-something) or making minor updates on a user branch, but generally this would be a good pattern to follow for organizational purposes. It is also crucial for generating easily understandable release notes where each item should correspond to a resolved ticket.

I believe there may be some confusion, the other 5 PR's are the related ticket work for DM-41063 are in other packages. We are changing how the edge flag works and what it means across multiple packages to make it more specific when we filter the trailed sources. Naturally, this means that the docstring for the flag needs to be updated as it no longer is accurate since it now no longer indicates off image trails. If you still feel this requires a user branch, I can do that, but this is directly related to the edge flag change and migration in ticket DM-41063.

[JeremyMcCormick]

Holding temporarily until I understand if this is actually resolving the referenced ticket, since the original PR had actual changes to the schema that added additional columns.

Don't these columns still need to be added to the schema to resolve the ticket? Or was it decided not to include them?

[JeremyMcCormick]

@bsmartradio indicated that the additional columns on the schemas were no longer needed and this PR resolves the work for the ticket on this repo.

[gpdf]

@bsmartradio We just noticed that the description change in this ticket was applied only to imsim.yaml and not to the same DiaSource.trail_flag_edge attribute in apdb.yaml. Was this an oversight?