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

Assert in Node that PhetioObject keys aren't provided to mutate more than once #1089

Open
zepumph opened this issue Oct 14, 2020 · 1 comment

Comments

@zepumph
Copy link
Member

zepumph commented Oct 14, 2020

@samreid and I felt like this would be the most complete to check in Node.initializePhetioObject. Something like:

assert && assert( _.intersection( PhetioObject.DEFAULTS, Object.keys( config)).length === 0 ) );`

I was a little concerned about performance, perhaps we can improve this a little bit, though I'm not sure how.

This is coming from work over in #1046.

@zepumph zepumph self-assigned this Oct 14, 2020
@zepumph zepumph removed their assignment Mar 3, 2023
@brent-phet
Copy link

FYI - Asked asked ChatGPT how it would label this issue for the board, and it actually offered optimization ideas first:

"To potentially improve the performance, you might consider the following approaches:

Optimize the Intersection Check:
The current implementation uses _.intersection, which could be less efficient if the objects have many keys. A more performant approach might involve a direct loop over the keys of one object and checking their existence in the other.

Limit the Check to Development Mode:
If the performance impact is significant, consider running this assertion only in development or debug mode. This way, you can catch potential issues during development without affecting the performance in production.

Cache Common Computations:
If there are aspects of the check that are invariant (do not change over time), consider caching them. For example, if PhetioObject.DEFAULTS is static, you could preprocess its keys into a more efficient data structure for quick lookup.

Use a More Efficient Data Structure:
Depending on the size of PhetioObject.DEFAULTS and config, it might be more efficient to use a data structure like a Set for quicker lookups.

A possible optimized version could look like this:

assert && assert((() => {
    const defaultsKeys = new Set(Object.keys(PhetioObject.DEFAULTS));
    return !Object.keys(config).some(key => defaultsKeys.has(key));
})());

This code creates a Set of keys from PhetioObject.DEFAULTS and then checks if any key in config is in that Set, returning true if there's no overlap. The use of Set could offer faster lookups compared to array-based intersection checks, especially if the number of keys is large."

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

No branches or pull requests

2 participants