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

Refactor, fix, and optimizer filters/rules #1794

Open
wants to merge 105 commits into
base: master
Choose a base branch
from

Conversation

dsblank
Copy link
Member

@dsblank dsblank commented Nov 22, 2024

This PR does three things:

  1. Unrolls the recursive call of filter.apply(), and splits out single checks into filter.apply_to_one()
  2. Uses JSON data where possible, passing the data rather than objects. If an object is needed, it will not be recreated more than once.
  3. Adds an optimizer based on rule.map sets

@dsblank
Copy link
Member Author

dsblank commented Dec 3, 2024

The only downside is that we leak the internal representation of the objects into the higher layers of code.

For many attributes of an object, it isn't so much "internal" now that we are switching to JSON. For example, this is the Person object API to get the parent family list:

person.parent_family_list # object attribute
person.get_parent_family_handle_list() # function call

With the above DataDict (with some additional code) with new JSON, it could look like (where person is really JSON data with a wrapper):

person.parent_family_list  # JSON access
person.get_parent_family_handle_list() # function call, instantiates object

The new version would hide the fact that the function call creates the object (but just once). So because the JSON data mirrors the actual attribute names, it doesn't have to change at all.

@dsblank dsblank changed the base branch from dsb/depickle to master December 8, 2024 15:27
@dsblank
Copy link
Member Author

dsblank commented Dec 11, 2024

Created a PR #1824 to explore attribute access of the JSON dict.

@dsblank
Copy link
Member Author

dsblank commented Dec 14, 2024

I'll update this PR to use new DataDict attribute access to dict. Which will actually revert most of the data access to what it is in master.

@dsblank
Copy link
Member Author

dsblank commented Dec 19, 2024

@Nick-Hall, I want to refactor this PR based on the DataDict from #1824. Should I change the base branch to #1824, or wait until it is merged?

@Nick-Hall
Copy link
Member

I have reviewed and merged PR #1824. Normally we would wait longer to give people longer to comment, but in this case the PR was related to existing work so I made an exception.

@dsblank
Copy link
Member Author

dsblank commented Dec 21, 2024

Thank you!

@dsblank dsblank requested review from stevenyoungs and Nick-Hall and removed request for stevenyoungs December 22, 2024 21:21
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