Skip to content

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