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

Fix dict filter by removing items, Fix tests for v0.3 compat #227

Closed
wants to merge 2 commits into from

Conversation

ranjanan
Copy link
Contributor

@ranjanan ranjanan commented Feb 3, 2016

Fix the function filter!() by removing items(d)

@ranjanan
Copy link
Contributor Author

ranjanan commented Feb 3, 2016

This should fix #226

@ranjanan
Copy link
Contributor Author

ranjanan commented Feb 3, 2016

@tkelman Could you please help with code review?

@ranjanan
Copy link
Contributor Author

ranjanan commented Feb 3, 2016

The Appveyor build fails because of this. There needs to be an @compat there for all the UInt8's. But I'm curious about why Appveyor checks against v0.3 while Travis checks against v0.4

@tkelman
Copy link
Contributor

tkelman commented Feb 3, 2016

The .travis.yml file here is still using the PPA for installing Julia, which does not make it very easy to install 0.3 any more. You can't use multiple language: settings, and it's a little more effort to install specific versions of Python than it is specific versions of Julia. The Travis file could be reformatted to download the generic Linux binaries (and osx dmg installers if osx is running) instead of the PPA to restore testing against 0.3.

@ranjanan ranjanan changed the title Fix dict filter by removing items Fix dict filter by removing items, Fix tests for v0.3 compat Feb 3, 2016
@ranjanan ranjanan mentioned this pull request Feb 3, 2016
@ranjanan
Copy link
Contributor Author

ranjanan commented Feb 3, 2016

I see. Maybe I should modify Travis to do pull the v0.3 binaries and send a separate PR. Also, the above commit fixes #228 . Actually, I belive the problem is that this test fails on 32 bit Windows. Interestingly, this is a regression between the latest release and the current master. I used a Windows VM and Pkg.test() worked on the last release but breaks on master.

@stevengj
Copy link
Member

stevengj commented Feb 3, 2016

Doing for (k,v) in d uses the dictionary iterator, which calls PyDict_Next. The documentation for the latter function explicitly says "The dictionary p should not be mutated during iteration"

Hence, I don't think this change is a good idea; there must be a more reliable way to fix it.

@stevengj
Copy link
Member

stevengj commented Feb 3, 2016

Two options that I can see:

  • Replace items(d) (which must have gotten removed from Julia at some point) by e.g. collect(Tuple{PyObject,PyObject}, d). This unfortunately makes a copy of the data.
  • Do for (k,v) in d or similar, but instead of deleting the keys right away, push! them to a temporary array of keys to be deleted. This is a bit more memory-efficient for large dictionaries.

@stevengj
Copy link
Member

stevengj commented Feb 4, 2016

Fixed by 73c3db7

@stevengj stevengj closed this Feb 4, 2016
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.

None yet

3 participants