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

Ensure get_dynamic_item does not modify object #927

Merged
merged 1 commit into from Oct 12, 2016

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Oct 11, 2016

So this was a pretty nasty bug, turns out my usage of .map with clone=False was modifying the object causing various weird bugs. This PR is a bit of an ugly workaround updating the item in DynamicMaps using a map call that doesn't modify the object and updates it and a separate map call to actually get the item. Can't think of a better to solution to update composite objects (i.e. Layouts and AdjointLayout) containing dimensionless DynamicMaps since select won't work.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Oct 12, 2016

Would calling select without arguments having the same effect as [()] for dimensionless dynamicmaps help (or at least appear more consistent)?

Otherwise, I'm happy to merge although I have to admit I have difficulty following this code (because it is a complicated thing to do..not because I think it is badly written).

@philippjfr

This comment has been minimized.

Member

philippjfr commented Oct 12, 2016

Would calling select without arguments having the same effect as [()] for dimensionless dynamicmaps help (or at least appear more consistent)?

I would like it if there was some way of doing this via select but I currently don't see select without arguments working in that case because select ends up being called for all kinds of things, often without specifying any valid dimensions for that particular object. Let's merge for now, and I'll keep thinking about it, this PR fixes some fairly nasty bugs so I'd like it merged now. Let's revisit when we look at counter mode, which faces similar hairy issues as dimensionless DynamicMaps.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Oct 12, 2016

Sure. Merging!

@jlstevens jlstevens merged commit b9aa0f4 into master Oct 12, 2016

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.008%) to 73.044%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the dynamic_item_fix branch Oct 14, 2016

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