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

Dask ms profiling #114

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Dask ms profiling #114

wants to merge 11 commits into from

Conversation

smasoka
Copy link

@smasoka smasoka commented May 5, 2020

  • Tests added / passed

    $ py.test -v -s daskms/tests

    If the pep8 tests fail, the quickest way to correct
    this is to run autopep8 and then flake8 and
    pycodestyle to fix the remaining issues.

    $ pip install -U autopep8 flake8 pycodestyle
    $ autopep8 -r -i daskms
    $ flake8 daskms
    $ pycodestyle daskms
    
  • Fully documented, including HISTORY.rst for all changes
    and one of the docs/*-api.rst files for new API

    To build the docs locally:

    pip install -r requirements.readthedocs.txt
    cd docs
    READTHEDOCS=True make html
    

daskms/table_proxy.py Outdated Show resolved Hide resolved
daskms/table_proxy.py Outdated Show resolved Hide resolved
daskms/table_proxy.py Outdated Show resolved Hide resolved
daskms/table_proxy.py Outdated Show resolved Hide resolved
daskms/table_proxy.py Outdated Show resolved Hide resolved
daskms/table_proxy.py Outdated Show resolved Hide resolved

wrapper.calls = 0
wrapper.run_time = []
return wrapper
Copy link
Member

Choose a reason for hiding this comment

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

As _nolock_runner


wrapper.calls = 0
wrapper.run_time = []
return wrapper
Copy link
Member

Choose a reason for hiding this comment

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

As _writelock_runner

@@ -0,0 +1,227 @@
# -*- coding: utf-8 -*-

try:
Copy link
Member

Choose a reason for hiding this comment

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

Start the test file from scratch.

def test_function_profiling():
    from daskms.table_proxy import _function_runs

   # Look at the rest of the test cases that use xds_from_ms
   # Set this one's arguments up similarly
   # Use xds_from_ms to produce datasets
   # then call compute on the datasets or the dataset arrays
   # that should invoke your profiling code
   # Whose results you can inspect in _function_runs

@sjperkins sjperkins marked this pull request as draft May 7, 2020 09:41
table_keywords changes to class type 'tuple'. So I made a change to 
_put_keywords function to check for None.

Added print statements to trace table_keywors (debugging)
@smasoka smasoka marked this pull request as ready for review May 29, 2020 11:22
@smasoka
Copy link
Author

smasoka commented May 29, 2020

The test code from test_ms_read_and_update.py does what I was. I will add more functions to test other proxied_methods in a similar way.
I am not sure why table_keywords changes to class type tuple inside _put_keywords function?

@sjperkins
Copy link
Member

The test code from test_ms_read_and_update.py does what I was. I will add more functions to test other proxied_methods in a similar way.

OK, I can see that you've added a print function at the end of the test. I assume that this is what you're using to verify your results. Generally when one writes tests one should use assert statements on the output to verify correctness. Something like:

from numpy.testing import assert_array_equal, assert_array_almost_equal
assert x == 1                                  # Standard python equality
assert_array_equal(y, z)                # Test arrays are exactly equal
assert_array_almost_equal(y, z)   # Test arrays are minimally different from each other

I am not sure why table_keywords changes to class type tuple inside _put_keywords function?

Hmmmm, it shouldn't. Is a tuple being passed into xds_from_ms or xds_from_table?

xds_from_ms(..., table_keywords=('x', 'y'))

for instance.

@smasoka
Copy link
Author

smasoka commented Jun 2, 2020

Yes, I am using assert statements.
I was printing to see results for myself and also track table_keywords so you can see my confusion a bit. I'll do another commit end of the day

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