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

Improve column names for disp #2443

Merged
merged 6 commits into from
Nov 6, 2023

Conversation

LukasBeiske
Copy link
Contributor

  • This removes the disp_parameter_is_valid column, since it is identical to the disp_tel_is_valid column.
  • Doing this makes it possible to use the same prefix for both the DispContainer and the ReconstructedGeometryContainer, as filled by DispReconstructor.
  • As the DispContainer contains the output values of both the regressor (norm) and the classifier (sign) as a single value (sign * norm), I renamed this value to "parameter" to avoid confusion.

HOWEVER: DispReconstructor still creates all the geometry-related columns contained in ReconstructedGeometryContainer even though it only computes altitude and azimuth. But since the creation of such empty columns is not limited to the disp reconstructor, I think it would be better to address this in a different PR.

@maxnoe
Copy link
Member

maxnoe commented Oct 31, 2023

Related: #2228

We could have a ReconstructedDirectionContainer, a ReconstructedImpactContainer and a ReconstructedGeometryContainer that combines the first two for algorithms that do all on one go.

Copy link
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

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

The changes look good to me, but this requires a bump in the data model version,
an entry in the data model changelog and probably dropping support for reading older files.

@maxnoe
Copy link
Member

maxnoe commented Oct 31, 2023

And some tests and more code has to be adapted, see the test failures.

Tobychev
Tobychev previously approved these changes Nov 6, 2023
@maxnoe maxnoe merged commit 195a942 into cta-observatory:main Nov 6, 2023
11 checks passed
@LukasBeiske LukasBeiske deleted the better_disp_columns branch November 6, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants