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

Accessing the cartesian routine #98

Closed
oyamad opened this issue Nov 18, 2014 · 12 comments
Closed

Accessing the cartesian routine #98

oyamad opened this issue Nov 18, 2014 · 12 comments

Comments

@oyamad
Copy link
Member

oyamad commented Nov 18, 2014

Hi,
I wanted to use cartesian, but found it a bit tricky to access it.

Does not work:

import quantecon as qe
qe.cartesian([[0, 1], [0, 1], [0, 1]])

It raises AttributeError: 'module' object has no attribute 'cartesian'.
(The same goes for qe.cartesian.cartesian([[0, 1], [0, 1], [0, 1]]).)

Works:

import quantecon.cartesian
quantecon.cartesian.cartesian([[0, 1], [0, 1], [0, 1]])

Is this as intended?
It is not mentioned in the documentation.

@jstac
Copy link
Contributor

jstac commented Nov 18, 2014

@oyamad Thanks. It needed to be added to the __init__.py file. I've done that now and your example works. The change is in the repo. Please let me know if there are issues. If not please close this.

@jstac jstac closed this as completed Nov 18, 2014
@jstac
Copy link
Contributor

jstac commented Nov 18, 2014

Oops, closed it by accident. Open again.

@jstac jstac reopened this Nov 18, 2014
@oyamad
Copy link
Member Author

oyamad commented Nov 18, 2014

@jstac Many thanks!

One comment and one issue:

  1. It would be nice if this procedure (adding the module?/routine? name to __init__.py) will be documented somewhere in an instruction for developers.

  2. Regarding this routine itself, it changes the type of the inputs: I wanted to get product sets of integers for the input list of lists of integers (in order to use them for indices of a list), but the elements in the output are floats.
    Maybe this line is responsible for that?

    Or maybe the user is responsible for changing the type back to the original one by astype.

@jstac
Copy link
Contributor

jstac commented Nov 18, 2014

@oyamad Thanks for feedback. Please feel free to add a comment to the developers wiki on that point you mention:

https://github.com/QuantEcon/QuantEcon.py/wiki

@albop Any thoughts on point 2?

@albop
Copy link
Contributor

albop commented Nov 20, 2014

Just out of curiosity, I commented on this issue by mail but it doesn't show online. Did you receive it ?

@albop
Copy link
Contributor

albop commented Nov 20, 2014

Here is the content of the mail I sent. Please disregard it if you have already read it.

On point 2, @oyamad is perfectly right. It would be indeed be better if the cartesian set had the same type as the inputs. If the output is stored as a single array, there will still be the restriction that all list of nodes are assumed to be of the same type (i.e. no product of a set of integer by a set of floats), but that is probably a weaker restriction. One thing that we have to make sure of is, that the routine, responsible for performing the product is correctly compiled for the various types. I will make a pull request for that, just let me a few days.

About the initial question of whether cartesian should be accessible from the top-level namespace, I am not a big fan of the change since it seems that the function cartesian() will shadow the module cartesian. [currently qe.cartesian refers to the module]. Ultimately, the problem is the fact that the module contains a function with the same name, so maybe one of them would need to change. I am not sure about what the canonical naming scheme would be. Something that I like to see in the same module would be a julia-like repeat function that basically merges numpy.tile, and numpy.repeat. So I guess the module is the one that should be renamed. How would you call it ?

@jstac
Copy link
Contributor

jstac commented Nov 21, 2014

@albop I didn't receive this comment separate from your reposting. I'm not sure why. I checked my email at github and it's fine.

How about gridtools or grid_tools? (Underscores aren't conventional in Python module names but they have already crept in to the library and I find they improve readability.)

@oyamad
Copy link
Member Author

oyamad commented Nov 23, 2014

@albop Thanks!

Just to mention, the corresponding routine in itertools is called product.

@sglyon
Copy link
Member

sglyon commented Jan 28, 2015

Seems like this is resolved. Can anyone confirm and close?

@albop
Copy link
Contributor

albop commented Jan 28, 2015

The cartesian package has to be renamed to gridtools or grid_tools as @jstac has suggested. Both seem perfect to me.

@sglyon
Copy link
Member

sglyon commented Jan 28, 2015

I'd vote for gridtools.

@jstac
Copy link
Contributor

jstac commented Jan 28, 2015

Fine by me.

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

4 participants