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

helpers for datadict #392

Merged
merged 12 commits into from
May 31, 2023
Merged

Conversation

wpfff
Copy link
Contributor

@wpfff wpfff commented May 25, 2023

  • avoid deepcopy
  • meta-object for easier data access

@wpfff wpfff requested a review from marcosfrenkel May 26, 2023 02:13
@wpfff wpfff marked this pull request as ready for review May 26, 2023 02:14
@wpfff
Copy link
Contributor Author

wpfff commented May 26, 2023

@marcosfrenkel can you have a look?
Also -- where should the new data access object be documented?

two main changes:

  • deepcopying was in many places, and it's very redundant (we do it anyway in all plottr nodes). so i removed copying as default from pretty much anywhere in datadict, except where you'd really want it (like when returning the structure of the data)
  • there's a new attribute to datadict: data of field 'x' can now be accessed as data.d_.x -- this gives direct access to the values of field 'x'.

Copy link
Collaborator

@marcosfrenkel marcosfrenkel left a comment

Choose a reason for hiding this comment

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

Everything other than comment looks good, and even the comment is nothing too important.

@marcosfrenkel
Copy link
Collaborator

@wpfff, the documentation for datadicts currently live here and that is the repository where the new feature should be documented. I can update all of the sub-modules there so that the API sections of stuff shows the current code.

@wpfff
Copy link
Contributor Author

wpfff commented May 31, 2023

@marcosfrenkel i fixed the one thing you mentioned. ready to approve?

Copy link
Collaborator

@marcosfrenkel marcosfrenkel left a comment

Choose a reason for hiding this comment

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

Looks good!

@marcosfrenkel marcosfrenkel merged commit 40360bf into toolsforexperiments:master May 31, 2023
@wpfff wpfff deleted the feature/ddtools branch May 31, 2023 02:17
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.

2 participants