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

Another attempt I started at defining typed fields #1305

Closed
wants to merge 1 commit into from

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Apr 29, 2020

@kosack I saw you did something similar in #1301

For discussion

@kosack
Copy link
Contributor

kosack commented Apr 29, 2020

That is a bit cleaner way to implement the validation. But then in the writer I would have to test for which type of Field subclass it is, to know if I need to deal with the units, etc, right?

I may refactor it this way though, since it is a bit clearer. Currently my way is pretty similar, but with only one Field class, and fully backward compatible with how things are defined now, so I can apply it to selected fields without changing all at once. Of course, I could make Field subclasses to achieve the same as above (though with a more complex validation function still)

@maxnoe
Copy link
Member Author

maxnoe commented Apr 29, 2020

That is a bit cleaner way to implement the validation. But then in the writer I would have to test for which type of Field subclass it is, to know if I need to deal with the units, etc, right?

My Idea was, that yes, the writer checks which types of fields it has at hand, to be able to make very informed decisions on how the table schema should look like.

@maxnoe
Copy link
Member Author

maxnoe commented Apr 29, 2020

Yes, I think the subclasses way might be cleaner, as it will result in less complex validation functions and less complex code for simple fields in the table writer.

This basically reduced the complexity for the simple fields and only requires more advanced checks for more complex fields.

I think this will be needed in the end, e.g. for stuff like coordinate fields, where you want the writer to create two or three columns for e.g. AltAz coordinates but we want to use a SkyCoord in memory

@kosack
Copy link
Contributor

kosack commented Apr 30, 2020

Ok, perhaps let's do this in an incremental way, since I just want to work on the Container hierarchy first, and not the TableWriter. In #1301 I implement these features in a less nice way, but it doesn't require any changes so far to the current Containers or the TableWriters. We can then do a second pass to implement the nicer hierarchy given here once that is merged.

@moralejo
Copy link
Contributor

Hi, I have a question: do you have in mind to define something like "histogram" fields? I needed them in some work I am doing for data check in lstchain. The Histogram class in ctapipe did not seem to have much functionality (and could only be written as FITS).
So what I did in the end is to define fields which contain the arrays of counts (1-d or 2-d) and, in a different container, on a different table, define the binnings. The separation of the two is more efficient data-volume-wise, in case the binning is the same (field-wise) for all rows in a table. Would be nice to have something like that in ctapipe.

@maxnoe maxnoe closed this Oct 20, 2020
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.

3 participants