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

Making things "pythonic" #17

Closed
wants to merge 11 commits into from
Closed

Conversation

iliakur
Copy link
Contributor

@iliakur iliakur commented Aug 31, 2014

This is a biggish update from me (mostly addressing #10 and #11) and I would like to discuss a couple of things in it before you merge my changes into the master branch. Treat this not just as a finished product, but as a work in progress. Please do take advantage of commenting on specific lines (git lets you do that, hurrah!), I'm looking forward to your feedback.
I've worked on the following stuff:

  1. PUF file is read from a pickled binary file which is much much faster than using Pandas. What is read in is already a dictionary where every variable is a key and its PUF np.array column is the value. While generating the pickle file I saved the len() of the pandas DataFrame which came from puf.csv under the key 'PUF DIM'. This then gets used by Sameer's PUF() function to set the vars we're missing to arrays of zeros with the right length (same as other PUF vars). This length is also used in a couple of other functions and I'd like to talk about those in more detail, see my comments to (and in!) the source code below.
  2. Plan X and (optionally!) Plan Y can now be read in as JSON files. I've created a version of the Plan X JSON and will have to show it to Dan to finalize, but it's a copy of all the variables being defined in Sameer's code and the script runs fine with it, so I presume I haven't broken anything and we're fine.
  3. Some "pythony" things (based on documentation and this):
    • all tabs have been replaced with spaces (1 tab char = 4 space chars)
    • I've started removing underscores from variable names, but would like to finish that before we merge
    • Python doesn't require one to preallocate memory for an array, so I removed some np.zeros() calls, but would like to chat more about that with @SameerSarkar

namesCap = [str.upper(n) if str.isalpha(n) else n for n in names]
# This should be dealt with so that we don't keep referencing the same array
# simply find all uses of this and figure out if they're called for (mostly not)
DIMARRAY = np.zeros((139651,))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is sketchy (basically all subsequent np.zeros() arrays are just referencing this one), but it's here to purposefully draw our attention to cases where we've been using np.zeros() and make us revisit them critically.

@iliakur
Copy link
Contributor Author

iliakur commented Oct 23, 2014

This is stale, closing.

@iliakur iliakur closed this Oct 23, 2014
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