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

Initial Functions Module Thread. Do not merge #17

Closed
wants to merge 21 commits into from
Closed

Initial Functions Module Thread. Do not merge #17

wants to merge 21 commits into from

Conversation

JensGM
Copy link
Contributor

@JensGM JensGM commented Jul 6, 2020

No description provided.

@JensGM JensGM marked this pull request as ready for review July 14, 2020 16:46
xun/functions/dag.py Outdated Show resolved Hide resolved
Copy link

@achaikou achaikou left a comment

Choose a reason for hiding this comment

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

OK, looks like I emerged from this black hole. 👾

Flight is kinda ok now, dark matter [except for deep-and-no-copy-stuff] is mostly cracked.
Thank you for the writing docs (really appreciated) and giving me the opportunity to find typos and improvements! (half of the comments is about them 😂 )
Remember, that if something requires explanation to me, it might be useful to do so not only in the comments, but somewhere in the code also 😄
Plus tests, tests and more tests! 😹

This is very cool in general though 👍

And now it's your turn to suffer 😈

xun/functions/context.py Show resolved Hide resolved
README.md Show resolved Hide resolved
main()
```

Note that the `main` function defined here is optional. This project could either be run as is, or run using xun. To run the program using xun, run the following:

Choose a reason for hiding this comment

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

This is one of the very first lines users will read. So maybe explicitly mark the difference between running project as is and with using xun so it was clear what xun is.

Also, just calling xun without main won't print anything. As a user, where could I find the results or my computations?

@@ -14,3 +14,179 @@

https://en.wikipedia.org/wiki/Bagua
```

Choose a reason for hiding this comment

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

  1. One-liner even before tutorial stating what xun is in general would be good.

  2. Also, put a note about technical requirements. Meaning at least exactly which Python versions are supported (I think some of the attributes you use appear only from certain version)

```python
def fibonacci_number(n):
```
The function definition is just normal a python function definition.

Choose a reason for hiding this comment

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

  1. just (a) normal

  2. is it worth to separate it into its own category then?

xun/functions/program.py Show resolved Hide resolved
Comment on lines +295 to +298
def compile_function_graph_builder(context, function_name):
"""Compile a function graph builder

Make a graph builder from a context function, results are memoized
Copy link

Choose a reason for hiding this comment

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

You can be more helpful!

  1. is "graph_builder" actually a scheduler? (Complicated words frighten me!)
  2. is it important that it is compiled or can we make understanding this easier and just say "create/make"?

so something like "create_calls_scheduler(context, function)" ?

And update the return value. "sub-dag" doesn't help too much

return decomposed.assemble(decomposed.xun_graph).compile()


def build_function_graph(context, call):
Copy link

Choose a reason for hiding this comment

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

create_call_schedule to be similar to previous one and confuse everyone?
(schedule_call sounds nicer but it might imply that something is already sent for execution, which is wrong)

If you agree that this makes more sense, other methods might be renamed in similar manner.
"functions graph" just doesn't tell me much.

xun/functions/program.py Show resolved Hide resolved
xun/functions/transformations.py Show resolved Hide resolved
@JensGM JensGM mentioned this pull request Sep 22, 2020
@JensGM JensGM changed the title WIP Functions module Initial Functions Module Thread. Do not merge Sep 22, 2020
@JensGM JensGM closed this Jan 19, 2021
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

2 participants