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

Break export() down into 3 separate functions #44

Closed
rhiever opened this issue Dec 5, 2015 · 8 comments
Closed

Break export() down into 3 separate functions #44

rhiever opened this issue Dec 5, 2015 · 8 comments

Comments

@rhiever
Copy link
Contributor

rhiever commented Dec 5, 2015

export() is currently too large. Break it down into 3 functions based on the primary steps of the function:

  • Replace all of the mathematical operators with their results
  • Unroll the nested function calls into serial code
  • Replace the function calls with their corresponding Python code

This change will make it easier to unit test the export() function as well.

@pronojitsaha
Copy link
Contributor

@rhiever While I am working on this, can you provide some inputs on the 'type' and 'contents' of the following just to be sure
-exported_pipeline,
-pipeline_list, and
-operator_text

@rhiever
Copy link
Contributor Author

rhiever commented Dec 18, 2015

@pronojitsaha, it may be best to hold off on this issue until we see what affect @dmarx's refactor (see #43 (comment)) has on the export function. I suspect it will shrink the export function significantly.

@pronojitsaha
Copy link
Contributor

Ok. I see that @dmarx's implementation still has the first two functions in export i.e.

  • Replace all of the mathematical operators with their results
  • Unroll the nested function calls into serial code

Should I break that into separate functions, it will streamline export() further and then incorporate @dmarx's refactor on the third part i.e. 'Replace the function calls with their corresponding Python code'?

@dmarx
Copy link

dmarx commented Dec 18, 2015

@pronojitsaha: Sounds good to me

@pronojitsaha
Copy link
Contributor

@dmarx Ok..great! Will get it done tomorrow.

@rhiever
Copy link
Contributor Author

rhiever commented Dec 18, 2015

👍

@pronojitsaha
Copy link
Contributor

@rhiever While we await the integration of #63, it will be good to break the final part i.e. 'Replace the function calls with their corresponding Python code' into a separate function or a code that the main tpot file imports from for improved code readability and comprehensibility significantly (similar way that I did the first two). I already have a basic structure, if you want can create a PR for it and you can look at it?

@rhiever
Copy link
Contributor Author

rhiever commented Dec 23, 2015

Sounds good to me.

On Wednesday, December 23, 2015, PRONOjit Saha notifications@github.com
wrote:

@rhiever https://github.com/rhiever While we await the integration of
#63 #63, it will be good to break
the final part i.e. 'Replace the function calls with their corresponding
Python code' into a separate function or a code that the main tpot file
imports from for improved code readability and comprehensibility
significantly (similar way that I did the first two). I already have a
basic structure, if you want can create a PR for it and you can look at it?


Reply to this email directly or view it on GitHub
#44 (comment).

Randal S. Olson, Ph.D.
Postdoctoral Researcher, Institute for Biomedical Informatics
University of Pennsylvania

E-mail: rso@randalolson.com | Twitter: @randal_olson
https://twitter.com/randal_olson
http://www.randalolson.com

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

No branches or pull requests

3 participants