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

[lang] Support matrix initialization with a list of vectors #811

Merged
merged 35 commits into from Apr 18, 2020

Conversation

KLozes
Copy link
Collaborator

@KLozes KLozes commented Apr 17, 2020

Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this! I think this is a very useful feature. Two thoughts:

  • Maybe we can use a syntax like ti.Matrix(rows=[a, b, c])/ti.Matrix(cols=[a, b, c]) so that the matrix composition is clearer? I was unsure if the vectors are rows or columns at the first glance. This also prevents ti.Matrix(...) from being too smart.
  • Let's briefly document this fancy matrix creation approach in https://github.com/taichi-dev/taichi/blob/master/docs/linalg.rst#matrices so that users know what you have done here :-)

@KLozes
Copy link
Collaborator Author

KLozes commented Apr 17, 2020

I think the rows/cols is a good idea. Should we make it mandatory? And should it also be used for list of list style initialization?

@yuanming-hu
Copy link
Member

yuanming-hu commented Apr 17, 2020

I think the rows/cols is a good idea. Should we make it mandatory? And should it also be used for list of list style initialization?

Yeah I think we can make it mandatory for making matrices from vectors. For list of list, let's keep the old style, since changing that might break some code.

Btw, I had to do a force-push just now to fix a PR merging issue on the master branch - could you update your local master branch, and rebase (or cherrypick) commit 919bfdb based on it, and then (force-)update your vec_to_matrix branch? Sorry about the trouble...
I fixed that. Please simply git fetch and then git reset --hard 95c55bce3410386037c53a27e0837fbf937e8950 on your branch. Sorry about that!

@archibate
Copy link
Collaborator

archibate commented Apr 18, 2020

Sorry about the confusing commits! To remove them:

git checkout master
git reset --hard origin/master
git pull --set-upstream origin master
git reset --hard origin/master
git checkout vector_to_matrix
git rebase -i master

Then you'll see:

pick xxx [glsl] xxx # delete this line!
pick xxx [glsl] yyy # delete this line!
pick xxx [lang] zzz
pick xxx [lang] www

Please delete those lines that are not related to this PR, then save and quit (:wq).

@yuanming-hu
Copy link
Member

Sorry about the confusing commits!

That was my fault - when merging #785 I accidentally chose Create a merge commit instead of Squash and merge. Then I had to use a force push to fix that, leading to the divergence here.

To prevent this from happening again, I have disabled Create a merge commit and Rebase and merge in this repo...

@KLozes KLozes requested a review from yuanming-hu April 18, 2020 15:56
@KLozes
Copy link
Collaborator Author

KLozes commented Apr 18, 2020

oh no. lots and lots of test failing, not just test_ad_atomic

test_ad_atomic.py F                                                                                                 [100%]

======================================================== FAILURES =========================================================
_____________________________________________________ test_ad_reduce ______________________________________________________

>   ???

/home/klozes/Documents/software/taichi/tests/python/test_ad_atomic.py:27: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../python/taichi/lang/kernel.py:484: in wrapped
    primal(*args, **kwargs)
../../python/taichi/lang/kernel.py:414: in __call__
    self.materialize(key=key, args=args, arg_features=arg_features)
../../python/taichi/lang/kernel.py:232: in materialize
    src = remove_indent(inspect.getsource(self.func))
/usr/lib/python3.6/inspect.py:973: in getsource
    lines, lnum = getsourcelines(object)
/usr/lib/python3.6/inspect.py:955: in getsourcelines
    lines, lnum = findsource(object)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

object = <function test_ad_reduce.<locals>.func at 0x7f50f83bdb70>

    def findsource(object):
        """Return the entire source file and starting line number for an object.
    
        The argument may be a module, class, method, function, traceback, frame,
        or code object.  The source code is returned as a list of all the lines
        in the file and the line number indexes a line in that list.  An OSError
        is raised if the source code cannot be retrieved."""
    
        file = getsourcefile(object)
        if file:
            # Invalidate cache if needed.
            linecache.checkcache(file)
        else:
            file = getfile(object)
            # Allow filenames in form of "<something>" to pass through.
            # `doctest` monkeypatches `linecache` module to enable
            # inspection, so let `linecache.getlines` to be called.
            if not (file.startswith('<') and file.endswith('>')):
                raise OSError('source code not available')
    
        module = getmodule(object, file)
        if module:
            lines = linecache.getlines(file, module.__dict__)
        else:
            lines = linecache.getlines(file)
        if not lines:
>           raise OSError('could not get source code')
E           OSError: could not get source code

/usr/lib/python3.6/inspect.py:786: OSError

@KLozes
Copy link
Collaborator Author

KLozes commented Apr 18, 2020

oh wait I see why its failing. for some reason the path to the test.py is wrong. but not for all tests

Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

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

Thank you so much! Now it LGTM except for a few minor nits.

Thanks for improving the documentation of both linalg (which should more or less be renamed matrix now...) and vector! This feature will be very useful.

docs/linalg.rst Outdated Show resolved Hide resolved
docs/linalg.rst Outdated Show resolved Hide resolved
docs/linalg.rst Outdated Show resolved Hide resolved
docs/vector.rst Outdated Show resolved Hide resolved
python/taichi/lang/matrix.py Show resolved Hide resolved
@KLozes
Copy link
Collaborator Author

KLozes commented Apr 18, 2020

bug fixed. all test seem to be running fine. I just had to delete the pycache in the test directory. I change the local repo name last week and I guess the cached files remembered the old name.

Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

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

Everything LGTM! Thanks!

@yuanming-hu yuanming-hu merged commit d3a5610 into taichi-dev:master Apr 18, 2020
@KLozes KLozes deleted the vec_to_matrix branch April 18, 2020 19:54
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

4 participants