Skip to content

Conversation

@chrishavlin
Copy link
Contributor

@chrishavlin chrishavlin commented Jul 21, 2025

PR Summary

This PR moves the derived_field_list to an attribute within the dataset's field_info container to help with #5214 . I have a follow up with the same change for the field_list, but that's much more invasive, so wanted to check how this smaller changeset does in the test suite (but the change here can be merged on its own).

@chrishavlin chrishavlin added enhancement Making something better refactor improve readability, maintainability, modularity workshop-2025 labels Jul 21, 2025
def derived_field_list(self):
self.index
return self.field_info.derived_field_list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just occurred to me that I should add a setter here for the sake of downstream frontends.

@chrishavlin chrishavlin marked this pull request as draft July 21, 2025 18:55
@jzuhone
Copy link
Contributor

jzuhone commented Jul 21, 2025

Broadly I like this idea--wondering if there are any potential performance changes related to accessing it from the FieldInfoContainer instead of the Dataset, since it's used fairly often?

Copy link
Member

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

I like this as a consolidation of duties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Making something better refactor improve readability, maintainability, modularity workshop-2025

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants